diff --git a/coderd/chatfiles/mime.go b/coderd/chatfiles/mime.go new file mode 100644 index 0000000000..f9f7f8b6b3 --- /dev/null +++ b/coderd/chatfiles/mime.go @@ -0,0 +1,250 @@ +package chatfiles + +import ( + "bytes" + "encoding/json" + "encoding/xml" + "maps" + "mime" + "path/filepath" + "slices" + "strings" + "unicode" + + "github.com/gabriel-vasile/mimetype" + "golang.org/x/xerrors" +) + +const MaxStoredFileNameBytes = 255 + +var ( + // ErrStoredFileNameRequired indicates that a durable file name is empty + // after normalization. + ErrStoredFileNameRequired = xerrors.New("stored file name is required") + + // ErrUnsupportedStoredFileType indicates that classified file bytes do not + // map to an allowed durable file type. + ErrUnsupportedStoredFileType = xerrors.New("unsupported attachment type") + + utf8BOM = []byte{0xEF, 0xBB, 0xBF} + + allowedStoredMediaTypes = map[string]struct{}{ + "image/png": {}, + "image/jpeg": {}, + "image/gif": {}, + "image/webp": {}, + "text/plain": {}, + "text/markdown": {}, + "text/csv": {}, + "application/json": {}, + "application/pdf": {}, + } + + recordingArtifactMediaTypes = map[string]struct{}{ + "video/mp4": {}, + "image/jpeg": {}, + } +) + +// DetectMediaType detects the base media type of the given file contents. +func DetectMediaType(data []byte) string { + return BaseMediaType(mimetype.Detect(data).String()) +} + +// BaseMediaType strips parameters from a media type. +func BaseMediaType(mediaType string) string { + if parsed, _, err := mime.ParseMediaType(mediaType); err == nil { + return parsed + } + return mediaType +} + +// AllowedStoredMediaTypesString returns the supported durable chat file media +// types as a comma-separated list. +func AllowedStoredMediaTypesString() string { + return strings.Join(slices.Sorted(maps.Keys(allowedStoredMediaTypes)), ", ") +} + +// IsAllowedStoredMediaType reports whether the media type is supported for +// durable chat file storage. +func IsAllowedStoredMediaType(mediaType string) bool { + _, ok := allowedStoredMediaTypes[BaseMediaType(mediaType)] + return ok +} + +// IsInlineRenderableStoredMediaType reports whether a stored chat file may be +// served with Content-Disposition: inline. PDFs remain storable but +// download-only because browser PDF viewers have a broader active-content +// attack surface than the other media types we allow inline. +func IsInlineRenderableStoredMediaType(mediaType string) bool { + mediaType = BaseMediaType(mediaType) + if !IsAllowedStoredMediaType(mediaType) { + return false + } + return mediaType != "application/pdf" +} + +// NormalizeStoredFileName trims surrounding whitespace, strips control +// characters, and truncates the name to the durable storage byte limit +// without splitting UTF-8 runes. +func NormalizeStoredFileName(name string) string { + name = strings.Map(func(r rune) rune { + if unicode.IsControl(r) { + return -1 + } + return r + }, name) + name = strings.TrimSpace(name) + return truncateUTF8Bytes(name, MaxStoredFileNameBytes) +} + +// PrepareStoredFile normalizes the display name, rejects empty normalized +// names, and classifies the file bytes using detectName when provided, so +// callers can preserve subtype detection even when the user-facing filename is +// overridden. +func PrepareStoredFile(name, detectName string, data []byte) (storedName, mediaType string, err error) { + storedName = NormalizeStoredFileName(name) + if storedName == "" { + return "", "", ErrStoredFileNameRequired + } + if strings.TrimSpace(detectName) == "" { + detectName = storedName + } + mediaType = ClassifyStoredMediaType(detectName, data) + if !IsAllowedStoredMediaType(mediaType) { + return "", "", xerrors.Errorf("%w %q", ErrUnsupportedStoredFileType, mediaType) + } + return storedName, mediaType, nil +} + +// PrepareRecordingArtifact normalizes the recording artifact name, rejects +// empty normalized names, and verifies that the bytes match the expected +// recording media type. +func PrepareRecordingArtifact(name, expectedMediaType string, data []byte) (storedName, mediaType string, err error) { + expectedMediaType = BaseMediaType(expectedMediaType) + if _, ok := recordingArtifactMediaTypes[expectedMediaType]; !ok { + return "", "", xerrors.Errorf("unsupported recording artifact type %q", expectedMediaType) + } + + storedName = NormalizeStoredFileName(name) + if storedName == "" { + return "", "", ErrStoredFileNameRequired + } + mediaType = DetectMediaType(data) + if mediaType != expectedMediaType { + return "", "", xerrors.Errorf("recording artifact type mismatch: expected %q, detected %q", expectedMediaType, mediaType) + } + return storedName, mediaType, nil +} + +// IsCompatibleUploadMediaType reports whether an upload request that declared +// declaredMediaType may be stored as storedMediaType after byte +// classification. Exact matches are always compatible; the compatibility +// table only covers explicit refinements like text/plain uploads that safely +// store as richer text subtypes. +func IsCompatibleUploadMediaType(declaredMediaType, storedMediaType string) bool { + declaredMediaType = BaseMediaType(declaredMediaType) + storedMediaType = BaseMediaType(storedMediaType) + + if declaredMediaType == storedMediaType { + return true + } + if declaredMediaType != "text/plain" { + return false + } + + switch storedMediaType { + case "text/markdown", "text/csv", "application/json": + return true + default: + return false + } +} + +// HasSVGRootElement reports whether the provided file bytes decode to an SVG +// root element. This catches SVG content even when generic sniffers classify it +// as text or XML. +func HasSVGRootElement(data []byte) bool { + data = bytes.TrimPrefix(data, utf8BOM) + if len(data) == 0 { + return false + } + + decoder := xml.NewDecoder(bytes.NewReader(data)) + for { + token, err := decoder.Token() + if err != nil { + return false + } + + switch token := token.(type) { + case xml.ProcInst, xml.Directive, xml.Comment: + continue + case xml.CharData: + if len(bytes.TrimSpace(token)) == 0 { + continue + } + return false + case xml.StartElement: + return strings.EqualFold(token.Name.Local, "svg") + default: + return false + } + } +} + +// ClassifyStoredMediaType returns the media type that durable chat storage +// would use for the given filename and bytes. Unsupported or blocked content is +// returned as its detected media type so callers can report the specific type. +func ClassifyStoredMediaType(name string, data []byte) string { + if HasSVGRootElement(data) { + return "image/svg+xml" + } + + mediaType := DetectMediaType(data) + switch mediaType { + case "image/png", "image/jpeg", "image/gif", "image/webp", + "text/markdown", "text/csv", "application/json", + "application/pdf", "application/xml", "text/xml": + return mediaType + case "text/plain": + return refineTextMediaType(name, data) + default: + if strings.HasPrefix(mediaType, "text/") { + return "text/plain" + } + return mediaType + } +} + +func refineTextMediaType(name string, data []byte) string { + switch strings.ToLower(filepath.Ext(name)) { + case ".json": + if json.Valid(data) { + return "application/json" + } + case ".md", ".markdown": + return "text/markdown" + case ".csv": + return "text/csv" + } + return "text/plain" +} + +func truncateUTF8Bytes(value string, maxBytes int) string { + if maxBytes <= 0 || value == "" { + return "" + } + if len(value) <= maxBytes { + return value + } + + cut := 0 + for idx := range value { + if idx > maxBytes { + break + } + cut = idx + } + return value[:cut] +} diff --git a/coderd/chatfiles/mime_test.go b/coderd/chatfiles/mime_test.go new file mode 100644 index 0000000000..bff79e3e13 --- /dev/null +++ b/coderd/chatfiles/mime_test.go @@ -0,0 +1,345 @@ +package chatfiles_test + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/chatfiles" +) + +func TestDetectMediaType_WebP(t *testing.T) { + t.Parallel() + + data := append([]byte("RIFF"), []byte{0x24, 0x00, 0x00, 0x00}...) + data = append(data, []byte("WEBPVP8 ")...) + require.Equal(t, "image/webp", chatfiles.DetectMediaType(data)) +} + +func TestClassifyStoredMediaType(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + fileName string + data []byte + want string + }{ + { + name: "PlainText", + fileName: "build.log", + data: []byte("build succeeded\n"), + want: "text/plain", + }, + { + name: "MarkdownFromExtension", + fileName: "notes.md", + data: []byte("# Release notes\n"), + want: "text/markdown", + }, + { + name: "CSVFromDetector", + fileName: "report.txt", + data: []byte("name,count\nwidgets,3\n"), + want: "text/csv", + }, + { + name: "JSONFromDetector", + fileName: "payload.txt", + data: []byte(`{"ok":true}`), + want: "application/json", + }, + { + name: "UppercaseJSONExtension", + fileName: "data.JSON", + data: []byte(`{"ok":true}`), + want: "application/json", + }, + { + name: "InvalidJSONExtensionFallsBackToPlainText", + fileName: "broken.json", + data: []byte("not json"), + want: "text/plain", + }, + { + name: "UppercaseMDExtension", + fileName: "NOTES.MD", + data: []byte("# Notes\n"), + want: "text/markdown", + }, + { + name: "PDF", + fileName: "report.pdf", + data: []byte("%PDF-1.7\n"), + want: "application/pdf", + }, + { + name: "BinaryOctetStream", + fileName: "data.bin", + data: []byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05}, + want: "application/octet-stream", + }, + { + name: "HTMLFallsBackToTextPlain", + fileName: "snippet.txt", + data: []byte("hello"), + want: "text/plain", + }, + { + name: "XMLStaysBlocked", + fileName: "note.xml", + data: []byte(`Tove`), + want: "text/xml", + }, + { + name: "SVGBlockedEvenWhenNamedText", + fileName: "notes.txt", + data: []byte(`Hello`), + want: "image/svg+xml", + }, + { + name: "MarkdownMentioningSVGStaysMarkdown", + fileName: "notes.md", + data: []byte("# SVG Example\n..."), + want: "text/markdown", + }, + { + name: "CSVMentioningSVGStaysCSV", + fileName: "report.csv", + data: []byte("name,icon\nlogo,\n"), + want: "text/csv", + }, + { + name: "TextMentioningSVGStaysPlainText", + fileName: "main.go", + data: []byte("package main\n// renders tags\n"), + want: "text/plain", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, chatfiles.ClassifyStoredMediaType(tt.fileName, tt.data)) + }) + } +} + +func TestPrepareStoredFile(t *testing.T) { + t.Parallel() + + t.Run("UsesDetectNameForSubtypeRefinement", func(t *testing.T) { + t.Parallel() + + name, mediaType, err := chatfiles.PrepareStoredFile( + "payload.txt", + "report.json", + []byte(`{"ok":true}`), + ) + require.NoError(t, err) + require.Equal(t, "payload.txt", name) + require.Equal(t, "application/json", mediaType) + }) + + t.Run("StripsControlCharactersAndTrimsExposedWhitespace", func(t *testing.T) { + t.Parallel() + + name, mediaType, err := chatfiles.PrepareStoredFile( + "\x00 release\t notes.txt \x00", + "release-notes.txt", + []byte("hello"), + ) + require.NoError(t, err) + require.Equal(t, "release notes.txt", name) + require.Equal(t, "text/plain", mediaType) + }) + + t.Run("RejectsEmptyNormalizedName", func(t *testing.T) { + t.Parallel() + + _, _, err := chatfiles.PrepareStoredFile( + " \r\n\t ", + "notes.txt", + []byte("hello"), + ) + require.ErrorIs(t, err, chatfiles.ErrStoredFileNameRequired) + }) + + t.Run("RejectsUnsupportedStoredFileType", func(t *testing.T) { + t.Parallel() + + _, _, err := chatfiles.PrepareStoredFile( + "evil.svg", + "evil.svg", + []byte(``), + ) + require.ErrorIs(t, err, chatfiles.ErrUnsupportedStoredFileType) + require.ErrorContains(t, err, "image/svg+xml") + }) + + t.Run("TruncatesNamesAtRuneBoundaries", func(t *testing.T) { + t.Parallel() + + name, _, err := chatfiles.PrepareStoredFile( + strings.Repeat("界", 100), + "notes.txt", + []byte("hello"), + ) + require.NoError(t, err) + require.Equal(t, strings.Repeat("界", 85), name) + require.Equal(t, 255, len(name)) + }) +} + +func TestPrepareRecordingArtifact(t *testing.T) { + t.Parallel() + + t.Run("MP4", func(t *testing.T) { + t.Parallel() + + name, mediaType, err := chatfiles.PrepareRecordingArtifact( + "recording.mp4", + "video/mp4", + []byte{0x00, 0x00, 0x00, 0x18, 'f', 't', 'y', 'p', 'm', 'p', '4', '2', 0x00, 0x00, 0x00, 0x00, 'm', 'p', '4', '1', 'i', 's', 'o', 'm'}, + ) + require.NoError(t, err) + require.Equal(t, "recording.mp4", name) + require.Equal(t, "video/mp4", mediaType) + }) + + t.Run("JPEG", func(t *testing.T) { + t.Parallel() + + name, mediaType, err := chatfiles.PrepareRecordingArtifact( + "thumbnail.jpg", + "image/jpeg", + []byte{0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10, 'J', 'F', 'I', 'F', 0x00, 0x01, 0x01, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00}, + ) + require.NoError(t, err) + require.Equal(t, "thumbnail.jpg", name) + require.Equal(t, "image/jpeg", mediaType) + }) + + t.Run("TypeMismatch", func(t *testing.T) { + t.Parallel() + + _, _, err := chatfiles.PrepareRecordingArtifact( + "recording.mp4", + "video/mp4", + []byte{0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10, 'J', 'F', 'I', 'F', 0x00, 0x01, 0x01, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00}, + ) + require.ErrorContains(t, err, "recording artifact type mismatch") + }) + + t.Run("RejectsEmptyNormalizedName", func(t *testing.T) { + t.Parallel() + + _, _, err := chatfiles.PrepareRecordingArtifact( + " \r\n\t ", + "video/mp4", + []byte{0x00, 0x00, 0x00, 0x18, 'f', 't', 'y', 'p', 'm', 'p', '4', '2', 0x00, 0x00, 0x00, 0x00, 'm', 'p', '4', '1', 'i', 's', 'o', 'm'}, + ) + require.ErrorIs(t, err, chatfiles.ErrStoredFileNameRequired) + }) + + t.Run("UnsupportedExpectedType", func(t *testing.T) { + t.Parallel() + + _, _, err := chatfiles.PrepareRecordingArtifact( + "recording.webm", + "video/webm", + []byte("webm"), + ) + require.ErrorContains(t, err, "unsupported recording artifact type") + }) +} + +func TestIsCompatibleUploadMediaType(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + declared string + stored string + want bool + }{ + { + name: "ExactMatch", + declared: "text/plain", + stored: "text/plain", + want: true, + }, + { + name: "TextPlainRefinesToMarkdown", + declared: "text/plain", + stored: "text/markdown", + want: true, + }, + { + name: "TextPlainRefinesToCSV", + declared: "text/plain", + stored: "text/csv", + want: true, + }, + { + name: "TextPlainRefinesToJSON", + declared: "text/plain", + stored: "application/json", + want: true, + }, + { + name: "TextPlainDoesNotRefineToPNG", + declared: "text/plain", + stored: "image/png", + want: false, + }, + { + name: "JSONDoesNotRefineToPlainText", + declared: "application/json", + stored: "text/plain", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, chatfiles.IsCompatibleUploadMediaType(tt.declared, tt.stored)) + }) + } +} + +func TestIsAllowedStoredMediaType(t *testing.T) { + t.Parallel() + + require.True(t, chatfiles.IsAllowedStoredMediaType("text/plain; charset=utf-8")) + require.True(t, chatfiles.IsAllowedStoredMediaType("text/markdown")) + require.True(t, chatfiles.IsAllowedStoredMediaType("text/csv")) + require.True(t, chatfiles.IsAllowedStoredMediaType("application/json")) + require.True(t, chatfiles.IsAllowedStoredMediaType("application/pdf")) + require.True(t, chatfiles.IsAllowedStoredMediaType("image/png")) + require.False(t, chatfiles.IsAllowedStoredMediaType("image/svg+xml")) + require.False(t, chatfiles.IsAllowedStoredMediaType("image/avif")) + require.False(t, chatfiles.IsAllowedStoredMediaType("application/zip")) +} + +func TestIsInlineRenderableStoredMediaType(t *testing.T) { + t.Parallel() + + require.True(t, chatfiles.IsInlineRenderableStoredMediaType("text/plain; charset=utf-8")) + require.True(t, chatfiles.IsInlineRenderableStoredMediaType("text/markdown")) + require.True(t, chatfiles.IsInlineRenderableStoredMediaType("image/png")) + require.False(t, chatfiles.IsInlineRenderableStoredMediaType("application/pdf")) + require.False(t, chatfiles.IsInlineRenderableStoredMediaType("image/svg+xml")) +} + +func TestHasSVGRootElement(t *testing.T) { + t.Parallel() + + require.True(t, chatfiles.HasSVGRootElement([]byte(``))) + require.True(t, chatfiles.HasSVGRootElement([]byte("\xef\xbb\xbf"))) + require.False(t, chatfiles.HasSVGRootElement([]byte("not svg"))) + require.False(t, chatfiles.HasSVGRootElement([]byte("# SVG Example\n..."))) + require.False(t, chatfiles.HasSVGRootElement([]byte("name,icon\nlogo,\n"))) +} diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index 0205def848..b67ebbdfea 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -1,8 +1,6 @@ package coderd import ( - "bufio" - "bytes" "context" "database/sql" "encoding/json" @@ -14,7 +12,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "slices" "strconv" "strings" "sync" @@ -30,6 +27,7 @@ import ( "cdr.dev/slog/v3" "github.com/coder/coder/v2/agent/agentssh" "github.com/coder/coder/v2/coderd/audit" + "github.com/coder/coder/v2/coderd/chatfiles" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" @@ -3292,49 +3290,8 @@ func parseCompactionThresholdKey(key string) (uuid.UUID, error) { const ( // maxChatFileSize is the maximum size of a chat file upload (10 MB). maxChatFileSize = 10 << 20 - // maxChatFileName is the maximum length of an uploaded file name. - maxChatFileName = 255 ) -// allowedChatFileMIMETypes lists the content types accepted for chat -// file uploads. SVG is explicitly excluded because it can contain scripts. -var allowedChatFileMIMETypes = map[string]bool{ - "image/png": true, - "image/jpeg": true, - "image/gif": true, - "image/webp": true, - "text/plain": true, - "image/svg+xml": false, // SVG can contain scripts. -} - -func allowedChatFileMIMETypesStr() string { - var types []string - for t, allowed := range allowedChatFileMIMETypes { - if allowed { - types = append(types, t) - } - } - slices.Sort(types) - return strings.Join(types, ", ") -} - -var ( - webpMagicRIFF = []byte("RIFF") - webpMagicWEBP = []byte("WEBP") -) - -// detectChatFileType detects the MIME type of the given data. -// It extends http.DetectContentType with support for WebP, which -// Go's standard sniffer does not recognize. -func detectChatFileType(data []byte) string { - if len(data) >= 12 && - bytes.Equal(data[0:4], webpMagicRIFF) && - bytes.Equal(data[8:12], webpMagicWEBP) { - return "image/webp" - } - return http.DetectContentType(data) -} - //nolint:revive // get-return: revive assumes get* must be a getter, but this is an HTTP handler. func (api *API) getChatSystemPrompt(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -4114,61 +4071,24 @@ func (api *API) postChatFile(rw http.ResponseWriter, r *http.Request) { if mediaType, _, err := mime.ParseMediaType(contentType); err == nil { contentType = mediaType } - - if allowed, ok := allowedChatFileMIMETypes[contentType]; !ok || !allowed { + if !chatfiles.IsAllowedStoredMediaType(contentType) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Unsupported file type.", - Detail: fmt.Sprintf("Allowed types: %s.", allowedChatFileMIMETypesStr()), + Detail: fmt.Sprintf("Allowed types: %s.", chatfiles.AllowedStoredMediaTypesString()), }) return } + // Extract filename from Content-Disposition header if provided. + var filename string + if cd := r.Header.Get("Content-Disposition"); cd != "" { + if _, params, err := mime.ParseMediaType(cd); err == nil { + filename = params["filename"] + } + } + r.Body = http.MaxBytesReader(rw, r.Body, maxChatFileSize) - br := bufio.NewReader(r.Body) - - // Peek at the leading bytes to sniff the real content type - // before reading the entire body. - peek, peekErr := br.Peek(512) - if peekErr != nil && !errors.Is(peekErr, io.EOF) && !errors.Is(peekErr, bufio.ErrBufferFull) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Failed to read file from request.", - Detail: peekErr.Error(), - }) - return - } - - // Verify the actual content matches an allowed file type so that - // a client cannot spoof Content-Type to serve active content. - detected := detectChatFileType(peek) - if mediaType, _, err := mime.ParseMediaType(detected); err == nil { - detected = mediaType - } - if contentType == "text/plain" && strings.HasPrefix(detected, "text/") { - detected = "text/plain" - } - if allowed, ok := allowedChatFileMIMETypes[detected]; !ok || !allowed { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Unsupported file type.", - Detail: fmt.Sprintf("Allowed types: %s.", allowedChatFileMIMETypesStr()), - }) - return - } - // The mismatch check below is security-critical: it prevents a text - // body from being uploaded under an image Content-Type (or vice - // versa) now that both text/plain and image types are in the - // allowlist. Combined with the X-Content-Type-Options: nosniff - // header applied globally, this ensures browsers respect the - // stored MIME type. - if detected != contentType { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "File content type does not match Content-Type header.", - Detail: fmt.Sprintf("Header declared %q but file content was detected as %q.", contentType, detected), - }) - return - } - - // Read the full body now that we know the type is valid. - data, err := io.ReadAll(br) + data, err := io.ReadAll(r.Body) if err != nil { var maxBytesErr *http.MaxBytesError if errors.As(err, &maxBytesErr) { @@ -4185,27 +4105,43 @@ func (api *API) postChatFile(rw http.ResponseWriter, r *http.Request) { return } - // Extract filename from Content-Disposition header if provided. - var filename string - if cd := r.Header.Get("Content-Disposition"); cd != "" { - if _, params, err := mime.ParseMediaType(cd); err == nil { - filename = params["filename"] - if len(filename) > maxChatFileName { - // Truncate at rune boundary to avoid splitting - // multi-byte UTF-8 characters. - var truncated []byte - for _, r := range filename { - encoded := []byte(string(r)) - if len(truncated)+len(encoded) > maxChatFileName { - break - } - truncated = append(truncated, encoded...) - } - filename = string(truncated) - } + // Verify the actual content matches an allowed file type so that + // a client cannot spoof Content-Type to serve active content. + filename, detected, err := chatfiles.PrepareStoredFile(filename, filename, data) + if err != nil { + switch { + case errors.Is(err, chatfiles.ErrStoredFileNameRequired): + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Filename is required.", + Detail: "Provide a filename in the Content-Disposition header.", + }) + case errors.Is(err, chatfiles.ErrUnsupportedStoredFileType): + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Unsupported file type.", + Detail: fmt.Sprintf("Allowed types: %s.", chatfiles.AllowedStoredMediaTypesString()), + }) + default: + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid file.", + Detail: err.Error(), + }) } + return + } + // The compatibility check below is security-critical: it keeps exact + // media-type matching by default while allowing safe text/plain + // refinements such as JSON, CSV, and Markdown now that upload + // classification can return richer stored media types. Combined with + // the X-Content-Type-Options: nosniff header applied globally, this + // still prevents clients from smuggling binary or active content under + // a safer declared Content-Type. + if !chatfiles.IsCompatibleUploadMediaType(contentType, detected) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "File content type does not match Content-Type header.", + Detail: fmt.Sprintf("Header declared %q but file content was detected as %q.", contentType, detected), + }) + return } - chatFile, err := api.Database.InsertChatFile(ctx, database.InsertChatFileParams{ OwnerID: apiKey.UserID, OrganizationID: orgID, @@ -4252,10 +4188,14 @@ func (api *API) chatFileByID(rw http.ResponseWriter, r *http.Request) { } rw.Header().Set("Content-Type", chatFile.Mimetype) + disposition := "attachment" + if chatfiles.IsInlineRenderableStoredMediaType(chatFile.Mimetype) { + disposition = "inline" + } if chatFile.Name != "" { - rw.Header().Set("Content-Disposition", mime.FormatMediaType("inline", map[string]string{"filename": chatFile.Name})) + rw.Header().Set("Content-Disposition", mime.FormatMediaType(disposition, map[string]string{"filename": chatFile.Name})) } else { - rw.Header().Set("Content-Disposition", "inline") + rw.Header().Set("Content-Disposition", disposition) } rw.Header().Set("Cache-Control", "private, max-age=31536000, immutable") rw.Header().Set("Content-Length", strconv.Itoa(len(chatFile.Data))) @@ -4325,7 +4265,7 @@ func createChatInputFromParts( Detail: fmt.Sprintf("Failed to retrieve file for %s[%d].", fieldName, i), } } - content = append(content, codersdk.ChatMessageFile(part.FileID, chatFile.Mimetype)) + content = append(content, codersdk.ChatMessageFile(part.FileID, chatFile.Mimetype, chatFile.Name)) fileIDs = append(fileIDs, part.FileID) // file-reference parts carry inline code snippets, not uploaded // files. They have no FileID and are excluded from file tracking. diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index f8671b6f47..2e3af13415 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -6958,31 +6958,17 @@ func TestPostChatFile(t *testing.T) { require.NotEqual(t, uuid.Nil, resp.ID) }) - t.Run("Success/JPEG", func(t *testing.T) { + t.Run("MissingFilename", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) client := newChatClient(t) firstUser := coderdtest.CreateFirstUser(t, client.Client) - data := append([]byte{0xFF, 0xD8, 0xFF, 0xE0}, make([]byte, 64)...) - resp, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "image/jpeg", "test.jpg", bytes.NewReader(data)) - require.NoError(t, err) - require.NotEqual(t, uuid.Nil, resp.ID) - }) - - t.Run("Success/WebP", func(t *testing.T) { - t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) - client := newChatClient(t) - firstUser := coderdtest.CreateFirstUser(t, client.Client) - - // WebP: RIFF + 4-byte size + WEBP + padding. - data := append([]byte("RIFF"), make([]byte, 4)...) - data = append(data, []byte("WEBP")...) - data = append(data, make([]byte, 64)...) - resp, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "image/webp", "test.webp", bytes.NewReader(data)) - require.NoError(t, err) - require.NotEqual(t, uuid.Nil, resp.ID) + data := append([]byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A}, make([]byte, 64)...) + _, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "image/png", "", bytes.NewReader(data)) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Contains(t, sdkErr.Message, "Filename is required") + require.Contains(t, sdkErr.Detail, "Content-Disposition") }) t.Run("Success/TextPlain", func(t *testing.T) { @@ -6991,19 +6977,45 @@ func TestPostChatFile(t *testing.T) { client := newChatClient(t) firstUser := coderdtest.CreateFirstUser(t, client.Client) - data := []byte("This is a test paste.\nWith multiple lines.\n") + data := []byte(`This is a test paste. +With multiple lines. +`) resp, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "text/plain", "test.txt", bytes.NewReader(data)) require.NoError(t, err) require.NotEqual(t, uuid.Nil, resp.ID) }) + t.Run("Success/TextPlainRefinesToJSON", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + + resp, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "text/plain", "pasted-text.txt", bytes.NewReader([]byte(`{"ok":true}`))) + require.NoError(t, err) + require.NotEqual(t, uuid.Nil, resp.ID) + }) + + t.Run("Success/TextPlainRefinesToCSV", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + + resp, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "text/plain", "pasted-text.txt", bytes.NewReader([]byte(`name,count +widgets,3 +`))) + require.NoError(t, err) + require.NotEqual(t, uuid.Nil, resp.ID) + }) + t.Run("UnsupportedContentType", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) client := newChatClient(t) firstUser := coderdtest.CreateFirstUser(t, client.Client) - _, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "application/pdf", "test.pdf", bytes.NewReader([]byte("%PDF-1.7"))) + _, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "application/zip", "test.zip", bytes.NewReader([]byte("PK"))) requireSDKError(t, err, http.StatusBadRequest) }) @@ -7017,18 +7029,6 @@ func TestPostChatFile(t *testing.T) { requireSDKError(t, err, http.StatusBadRequest) }) - t.Run("ContentSniffingRejects", func(t *testing.T) { - t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) - client := newChatClient(t) - firstUser := coderdtest.CreateFirstUser(t, client.Client) - - // Header says PNG but body is plain text. - _, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "image/png", "test.png", bytes.NewReader([]byte("hello world"))) - sdkErr := requireSDKError(t, err, http.StatusBadRequest) - require.Contains(t, sdkErr.Message, "does not match") - }) - t.Run("ContentSniffingRejectsPNGAsText", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) @@ -7051,13 +7051,25 @@ func TestPostChatFile(t *testing.T) { sdkErr := requireSDKError(t, err, http.StatusBadRequest) require.Contains(t, sdkErr.Message, "does not match") }) + + t.Run("ContentSniffingRejectsPlainTextAsJSON", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + + _, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "application/json", "payload.json", bytes.NewReader([]byte("not actually json"))) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Contains(t, sdkErr.Message, "does not match") + }) + t.Run("TooLarge", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) client := newChatClient(t) firstUser := coderdtest.CreateFirstUser(t, client.Client) - // 10 MB + 1 byte, with valid PNG header to pass MIME check. + // 10 MB + 1 byte, with valid PNG header to pass media type check. data := make([]byte, 10<<20+1) copy(data, []byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A}) _, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "image/png", "test.png", bytes.NewReader(data)) @@ -7070,7 +7082,9 @@ func TestPostChatFile(t *testing.T) { client := newChatClient(t) firstUser := coderdtest.CreateFirstUser(t, client.Client) - data := []byte("\n

Paste me as plain text.

\n") + data := []byte(` +

Paste me as plain text.

+`) resp, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "text/plain", "snippet.txt", bytes.NewReader(data)) require.NoError(t, err) require.NotEqual(t, uuid.Nil, resp.ID) @@ -7160,22 +7174,6 @@ func TestGetChatFile(t *testing.T) { require.Equal(t, data, got) }) - t.Run("Success/TextPlain", func(t *testing.T) { - t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) - client := newChatClient(t) - firstUser := coderdtest.CreateFirstUser(t, client.Client) - - data := []byte("This is a test paste.\nWith multiple lines.\n") - uploaded, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "text/plain", "test.txt", bytes.NewReader(data)) - require.NoError(t, err) - - got, contentType, err := client.GetChatFile(ctx, uploaded.ID) - require.NoError(t, err) - require.Equal(t, "text/plain", contentType) - require.Equal(t, data, got) - }) - t.Run("CacheHeaders", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) @@ -7197,6 +7195,29 @@ func TestGetChatFile(t *testing.T) { require.Contains(t, res.Header.Get("Content-Disposition"), "test.png") }) + t.Run("PDFServedAsAttachment", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + client := newChatClient(t) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + + uploaded, err := client.UploadChatFile(ctx, firstUser.OrganizationID, "application/pdf", "report.pdf", bytes.NewReader([]byte("%PDF-1.7\n"))) + require.NoError(t, err) + + res, err := client.Request(ctx, http.MethodGet, + fmt.Sprintf("/api/experimental/chats/files/%s", uploaded.ID), nil) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + require.Equal(t, "application/pdf", res.Header.Get("Content-Type")) + require.Equal(t, "nosniff", res.Header.Get("X-Content-Type-Options")) + + disposition, params, err := mime.ParseMediaType(res.Header.Get("Content-Disposition")) + require.NoError(t, err) + require.Equal(t, "attachment", disposition) + require.Equal(t, "report.pdf", params["filename"]) + }) + t.Run("LongFilename", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitLong) @@ -7213,7 +7234,7 @@ func TestGetChatFile(t *testing.T) { require.NoError(t, err) defer res.Body.Close() require.Equal(t, http.StatusOK, res.StatusCode) - // Filename should be truncated to maxChatFileName (255) bytes. + // Filename should be truncated to chatfiles.MaxStoredFileNameBytes (255) bytes. cd := res.Header.Get("Content-Disposition") require.Contains(t, cd, "inline") require.Contains(t, cd, strings.Repeat("a", 255)) diff --git a/coderd/x/chatd/attachments.go b/coderd/x/chatd/attachments.go new file mode 100644 index 0000000000..363479af5d --- /dev/null +++ b/coderd/x/chatd/attachments.go @@ -0,0 +1,63 @@ +package chatd + +import ( + "context" + + "charm.land/fantasy" + "github.com/google/uuid" + + "cdr.dev/slog/v3" + "github.com/coder/coder/v2/coderd/x/chatd/chatloop" + "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk" +) + +func buildAssistantPartsForPersist( + ctx context.Context, + logger slog.Logger, + assistantBlocks []fantasy.Content, + toolResults []fantasy.ToolResultContent, + step chatloop.PersistedStep, + toolNameToConfigID map[string]uuid.UUID, +) []codersdk.ChatMessagePart { + parts := make([]codersdk.ChatMessagePart, 0, len(assistantBlocks)+len(toolResults)) + for _, block := range assistantBlocks { + part := chatprompt.PartFromContent(block) + if part.ToolName != "" { + if configID, ok := toolNameToConfigID[part.ToolName]; ok { + part.MCPServerConfigID = uuid.NullUUID{UUID: configID, Valid: true} + } + } + if part.Type == codersdk.ChatMessagePartTypeToolCall && part.ToolCallID != "" && step.ToolCallCreatedAt != nil { + if ts, ok := step.ToolCallCreatedAt[part.ToolCallID]; ok { + part.CreatedAt = &ts + } + } + if part.Type == codersdk.ChatMessagePartTypeToolResult && part.ToolCallID != "" && step.ToolResultCreatedAt != nil { + if ts, ok := step.ToolResultCreatedAt[part.ToolCallID]; ok { + part.CreatedAt = &ts + } + } + parts = append(parts, part) + } + for _, tr := range toolResults { + attachments, err := chattool.AttachmentsFromMetadata(tr.ClientMetadata) + if err != nil { + logger.Warn(ctx, "skipping malformed tool attachment metadata", + slog.F("tool_name", tr.ToolName), + slog.F("tool_call_id", tr.ToolCallID), + slog.Error(err), + ) + continue + } + for _, attachment := range attachments { + parts = append(parts, codersdk.ChatMessageFile( + attachment.FileID, + attachment.MediaType, + attachment.Name, + )) + } + } + return parts +} diff --git a/coderd/x/chatd/attachments_test.go b/coderd/x/chatd/attachments_test.go new file mode 100644 index 0000000000..d3585ba505 --- /dev/null +++ b/coderd/x/chatd/attachments_test.go @@ -0,0 +1,136 @@ +package chatd //nolint:testpackage + +import ( + "context" + "testing" + "time" + + "charm.land/fantasy" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/x/chatd/chatloop" + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +func TestBuildAssistantPartsForPersist_PromotesToolAttachments(t *testing.T) { + t.Parallel() + + fileID := uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") + response := chattool.WithAttachments( + fantasy.NewTextResponse(`{"ok":true}`), + chattool.AttachmentMetadata{ + FileID: fileID, + MediaType: "image/png", + Name: "screenshot.png", + }, + ) + toolCallAt := time.Date(2026, time.April, 10, 0, 0, 0, 0, time.UTC) + + parts := buildAssistantPartsForPersist( + context.Background(), + testutil.Logger(t), + []fantasy.Content{fantasy.TextContent{Text: "Here is the screenshot."}}, + []fantasy.ToolResultContent{{ + ToolCallID: "call-1", + ToolName: "computer", + ClientMetadata: response.Metadata, + ProviderExecuted: false, + }}, + chatloop.PersistedStep{ + ToolCallCreatedAt: map[string]time.Time{ + "call-1": toolCallAt, + }, + }, + nil, + ) + + require.Len(t, parts, 2) + require.Equal(t, codersdk.ChatMessagePartTypeText, parts[0].Type) + require.Equal(t, "Here is the screenshot.", parts[0].Text) + require.Equal(t, codersdk.ChatMessagePartTypeFile, parts[1].Type) + require.True(t, parts[1].FileID.Valid) + require.Equal(t, fileID, parts[1].FileID.UUID) + require.Equal(t, "image/png", parts[1].MediaType) + require.Equal(t, "screenshot.png", parts[1].Name) +} + +func TestBuildAssistantPartsForPersist_PromotesProposePlanAttachment(t *testing.T) { + t.Parallel() + + fileID := uuid.MustParse("bbbbbbbb-cccc-dddd-eeee-ffffffffffff") + response := chattool.WithAttachments( + fantasy.NewTextResponse(`{"ok":true,"kind":"plan"}`), + chattool.AttachmentMetadata{ + FileID: fileID, + MediaType: "text/markdown", + Name: "PLAN.md", + }, + ) + + parts := buildAssistantPartsForPersist( + context.Background(), + testutil.Logger(t), + []fantasy.Content{fantasy.TextContent{Text: "Here is the proposed plan."}}, + []fantasy.ToolResultContent{{ + ToolCallID: "call-plan", + ToolName: "propose_plan", + ClientMetadata: response.Metadata, + }}, + chatloop.PersistedStep{}, + nil, + ) + + require.Len(t, parts, 2) + require.Equal(t, codersdk.ChatMessagePartTypeText, parts[0].Type) + require.Equal(t, "Here is the proposed plan.", parts[0].Text) + require.Equal(t, codersdk.ChatMessagePartTypeFile, parts[1].Type) + require.True(t, parts[1].FileID.Valid) + require.Equal(t, fileID, parts[1].FileID.UUID) + require.Equal(t, "text/markdown", parts[1].MediaType) + require.Equal(t, "PLAN.md", parts[1].Name) +} + +func TestBuildAssistantPartsForPersist_InvalidAttachmentMetadataSkipsOnlyBrokenResult(t *testing.T) { + t.Parallel() + + goodFileID := uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") + goodResponse := chattool.WithAttachments( + fantasy.NewTextResponse(`{"ok":true}`), + chattool.AttachmentMetadata{ + FileID: goodFileID, + MediaType: "image/png", + Name: "good.png", + }, + ) + + parts := buildAssistantPartsForPersist( + context.Background(), + testutil.Logger(t), + []fantasy.Content{fantasy.TextContent{Text: "Here are the results."}}, + []fantasy.ToolResultContent{ + { + ToolCallID: "call-good", + ToolName: "computer", + ClientMetadata: goodResponse.Metadata, + }, + { + ToolCallID: "call-bad", + ToolName: "attach_file", + ClientMetadata: `{"attachments":[{"file_id":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}]}`, + }, + }, + chatloop.PersistedStep{}, + nil, + ) + + require.Len(t, parts, 2) + require.Equal(t, codersdk.ChatMessagePartTypeText, parts[0].Type) + require.Equal(t, codersdk.ChatMessagePartTypeFile, parts[1].Type) + require.True(t, parts[1].FileID.Valid) + require.Equal(t, goodFileID, parts[1].FileID.UUID) + require.Equal(t, "image/png", parts[1].MediaType) + require.Equal(t, "good.png", parts[1].Name) +} diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 3b658f433a..cf686a70fc 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -4713,6 +4713,7 @@ type rootChatToolsOptions struct { instruction *string skills *[]chattool.SkillMeta resolvePlanPath func(context.Context) (string, string, error) + storeFile chattool.StoreFileFunc isPlanModeTurn bool } @@ -4815,9 +4816,7 @@ func (p *Server) appendRootChatTools( GetWorkspaceConn: opts.workspaceCtx.getWorkspaceConn, ResolvePlanPath: opts.resolvePlanPath, IsPlanTurn: opts.isPlanModeTurn, - StoreFile: func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error) { - return p.storePlanSnapshotFile(ctx, opts.workspaceCtx, name, mediaType, data) - }, + StoreFile: opts.storeFile, })) } @@ -4826,59 +4825,6 @@ func (p *Server) appendRootChatTools( }, opts.modelConfigID)...) } -func (p *Server) storePlanSnapshotFile( - ctx context.Context, - workspaceCtx *turnWorkspaceContext, - name string, - mediaType string, - data []byte, -) (uuid.UUID, error) { - chatSnapshot := workspaceCtx.currentChatSnapshot() - if !chatSnapshot.WorkspaceID.Valid { - return uuid.Nil, xerrors.New("no workspace is associated with this chat. Use the create_workspace tool to create one") - } - - ws, err := p.db.GetWorkspaceByID(ctx, chatSnapshot.WorkspaceID.UUID) - if err != nil { - return uuid.Nil, xerrors.Errorf("resolve workspace: %w", err) - } - - row, err := p.db.InsertChatFile(ctx, database.InsertChatFileParams{ - OwnerID: chatSnapshot.OwnerID, - OrganizationID: ws.OrganizationID, - Name: name, - Mimetype: mediaType, - Data: data, - }) - if err != nil { - return uuid.Nil, xerrors.Errorf("insert chat file: %w", err) - } - - // Cap enforcement and dedup are handled atomically in SQL. - // rejected > 0 means the cap was exceeded. - rejected, err := p.db.LinkChatFiles(ctx, database.LinkChatFilesParams{ - ChatID: chatSnapshot.ID, - MaxFileLinks: int32(codersdk.MaxChatFileIDs), - FileIds: []uuid.UUID{row.ID}, - }) - switch { - case err != nil: - p.logger.Error(ctx, "failed to link file to chat", - slog.F("chat_id", chatSnapshot.ID), - slog.F("file_id", row.ID), - slog.Error(err), - ) - case rejected > 0: - p.logger.Warn(ctx, "file cap reached, file not linked to chat", - slog.F("chat_id", chatSnapshot.ID), - slog.F("file_id", row.ID), - slog.F("max_file_links", codersdk.MaxChatFileIDs), - ) - } - - return row.ID, nil -} - func appendDynamicTools( ctx context.Context, logger slog.Logger, @@ -5381,36 +5327,20 @@ func (p *Server) runChat( // Pre-marshal all content outside the transaction so the // FOR UPDATE lock is held only for the INSERT statements. // Marshaling is pure CPU work with no database dependency. + assistantParts := buildAssistantPartsForPersist( + ctx, + p.logger, + assistantBlocks, + toolResults, + step, + toolNameToConfigID, + ) + var assistantContent pqtype.NullRawMessage - if len(assistantBlocks) > 0 { - sdkParts := make([]codersdk.ChatMessagePart, 0, len(assistantBlocks)) - for _, block := range assistantBlocks { - part := chatprompt.PartFromContent(block) - if part.ToolName != "" { - if configID, ok := toolNameToConfigID[part.ToolName]; ok { - part.MCPServerConfigID = uuid.NullUUID{UUID: configID, Valid: true} - } - } - // Apply recorded timestamps so persisted - // tool-call parts carry accurate CreatedAt. - if part.Type == codersdk.ChatMessagePartTypeToolCall && part.ToolCallID != "" && step.ToolCallCreatedAt != nil { - if ts, ok := step.ToolCallCreatedAt[part.ToolCallID]; ok { - part.CreatedAt = &ts - } - } - // Provider-executed tool results appear in - // assistantBlocks rather than toolResults, - // so apply their timestamps here as well. - if part.Type == codersdk.ChatMessagePartTypeToolResult && part.ToolCallID != "" && step.ToolResultCreatedAt != nil { - if ts, ok := step.ToolResultCreatedAt[part.ToolCallID]; ok { - part.CreatedAt = &ts - } - } - sdkParts = append(sdkParts, part) - } - finalAssistantText = strings.TrimSpace(contentBlocksToText(sdkParts)) + if len(assistantParts) > 0 { + finalAssistantText = strings.TrimSpace(contentBlocksToText(assistantParts)) var marshalErr error - assistantContent, marshalErr = chatprompt.MarshalParts(sdkParts) + assistantContent, marshalErr = chatprompt.MarshalParts(assistantParts) if marshalErr != nil { return xerrors.Errorf("marshal assistant content: %w", marshalErr) } @@ -5443,7 +5373,7 @@ func (p *Server) runChat( totalCostMicros := chatcost.CalculateTotalCostMicros(usageForCost, callConfig.Cost) var insertedMessages []database.ChatMessage - err := p.db.InTx(func(tx database.Store) error { + if err := p.db.InTx(func(tx database.Store) error { // Verify this worker still owns the chat before // inserting messages. This closes the race where // EditMessage soft-deletes history and clears worker_id @@ -5536,8 +5466,7 @@ func (p *Server) runChat( } return nil - }, nil) - if err != nil { + }, nil); err != nil { return xerrors.Errorf("persist step transaction: %w", err) } @@ -5633,6 +5562,7 @@ func (p *Server) runChat( ) allowAskUserQuestion := isPlanModeTurn && isRootChat + storeChatAttachment := p.newStoreChatAttachmentFunc(&workspaceCtx) tools := []fantasy.AgentTool{ chattool.ReadFile(chattool.ReadFileOptions{ GetWorkspaceConn: workspaceCtx.getWorkspaceConn, @@ -5647,6 +5577,10 @@ func (p *Server) runChat( ResolvePlanPath: resolvePlanPathForTools, IsPlanTurn: isPlanModeTurn, }), + chattool.AttachFile(chattool.AttachFileOptions{ + GetWorkspaceConn: workspaceCtx.getWorkspaceConn, + StoreFile: storeChatAttachment, + }), chattool.Execute(chattool.ExecuteOptions{ GetWorkspaceConn: workspaceCtx.getWorkspaceConn, }), @@ -5676,6 +5610,7 @@ func (p *Server) runChat( instruction: &instruction, skills: &skills, resolvePlanPath: resolvePlanPathForTools, + storeFile: storeChatAttachment, isPlanModeTurn: isPlanModeTurn, }) } @@ -5744,7 +5679,9 @@ func (p *Server) runChat( desktopGeometry.DeclaredWidth, desktopGeometry.DeclaredHeight, workspaceCtx.getWorkspaceConn, + storeChatAttachment, quartz.NewReal(), + p.logger.Named("computer_use"), ), }) } diff --git a/coderd/x/chatd/chatprompt/chatprompt.go b/coderd/x/chatd/chatprompt/chatprompt.go index 70feb0aaa9..7c14dab8be 100644 --- a/coderd/x/chatd/chatprompt/chatprompt.go +++ b/coderd/x/chatd/chatprompt/chatprompt.go @@ -64,19 +64,10 @@ func ExtractFileID(raw json.RawMessage) (uuid.UUID, error) { return uuid.Parse(envelope.Data.FileID) } -// ConvertMessages converts persisted chat messages into LLM prompt -// messages without resolving file references from storage. Inline -// file data is preserved when present (backward compat). -func ConvertMessages( - messages []database.ChatMessage, -) ([]fantasy.Message, error) { - return ConvertMessagesWithFiles(context.Background(), messages, nil, slog.Logger{}) -} - // ConvertMessagesWithFiles converts persisted chat messages into LLM -// prompt messages, resolving file references via the provided -// resolver. When resolver is nil, file blocks without inline data -// are passed through as-is (same behavior as ConvertMessages). +// prompt messages, resolving user file references via the provided +// resolver. Persisted file references without bytes are omitted from +// the prompt instead of being replayed back to the model. func ConvertMessagesWithFiles( ctx context.Context, messages []database.ChatMessage, @@ -85,7 +76,8 @@ func ConvertMessagesWithFiles( ) ([]fantasy.Message, error) { // Phase 1: Parse all messages via ParseContent (→ SDK parts) // and collect file_id references from user messages for batch - // resolution. + // resolution. Assistant-side file attachments remain persisted chat + // metadata and are intentionally not replayed to the model. type parsedMessage struct { role codersdk.ChatMessageRole parts []codersdk.ChatMessagePart @@ -162,7 +154,7 @@ func ConvertMessagesWithFiles( }) case codersdk.ChatMessageRoleAssistant: fantasyParts := normalizeAssistantToolCallInputs( - partsToMessageParts(logger, pm.parts, resolved), + partsToMessageParts(logger, pm.parts, nil), ) for _, toolCall := range ExtractToolCalls(fantasyParts) { if toolCall.ToolCallID == "" || strings.TrimSpace(toolCall.ToolName) == "" { @@ -186,7 +178,7 @@ func ConvertMessagesWithFiles( } } } - toolParts := partsToMessageParts(logger, pm.parts, resolved) + toolParts := partsToMessageParts(logger, pm.parts, nil) if len(toolParts) == 0 { continue } @@ -1338,6 +1330,13 @@ func partsToMessageParts( } } } + if len(data) == 0 { + // File parts without bytes are persistence metadata, not + // prompt content. User uploads should have been resolved + // above; assistant tool attachments intentionally are not + // replayed into later model turns. + continue + } // Providers only accept a small set of MIME types in file // content blocks, typically images and PDFs. A synthetic // paste sent as a text/plain FilePart is dropped or rejected, diff --git a/coderd/x/chatd/chatprompt/chatprompt_test.go b/coderd/x/chatd/chatprompt/chatprompt_test.go index fc1d214d53..ac98ebe99e 100644 --- a/coderd/x/chatd/chatprompt/chatprompt_test.go +++ b/coderd/x/chatd/chatprompt/chatprompt_test.go @@ -44,7 +44,20 @@ func testMsgV1(role codersdk.ChatMessageRole, raw pqtype.NullRawMessage) databas } } -func TestConvertMessages_NormalizesAssistantToolCallInput(t *testing.T) { +func convertMessagesWithoutFiles(t *testing.T, messages []database.ChatMessage) []fantasy.Message { + t.Helper() + + prompt, err := chatprompt.ConvertMessagesWithFiles( + context.Background(), + messages, + nil, + slogtest.Make(t, nil), + ) + require.NoError(t, err) + return prompt +} + +func TestConvertMessagesWithFiles_NormalizesAssistantToolCallInput(t *testing.T) { t.Parallel() testCases := []struct { @@ -98,7 +111,7 @@ func TestConvertMessages_NormalizesAssistantToolCallInput(t *testing.T) { ) require.NoError(t, err) - prompt, err := chatprompt.ConvertMessages([]database.ChatMessage{ + prompt := convertMessagesWithoutFiles(t, []database.ChatMessage{ { Role: database.ChatMessageRoleAssistant, Visibility: database.ChatMessageVisibilityBoth, @@ -110,7 +123,6 @@ func TestConvertMessages_NormalizesAssistantToolCallInput(t *testing.T) { Content: toolContent, }, }) - require.NoError(t, err) require.Len(t, prompt, 2) require.Equal(t, fantasy.MessageRoleAssistant, prompt[0].Role) @@ -302,7 +314,7 @@ func TestInjectMissingToolResults_SkipsProviderExecuted(t *testing.T) { false, false, false, ) - prompt, err := chatprompt.ConvertMessages([]database.ChatMessage{ + prompt := convertMessagesWithoutFiles(t, []database.ChatMessage{ { Role: database.ChatMessageRoleAssistant, Visibility: database.ChatMessageVisibilityBoth, @@ -314,7 +326,6 @@ func TestInjectMissingToolResults_SkipsProviderExecuted(t *testing.T) { Content: localResult, }, }) - require.NoError(t, err) // Expected: assistant + tool(local result). No synthetic error // for the provider-executed tool call. @@ -404,7 +415,7 @@ func TestInjectMissingToolUses_DropsProviderExecutedOrphans(t *testing.T) { false, false, false, ) - prompt, err := chatprompt.ConvertMessages([]database.ChatMessage{ + prompt := convertMessagesWithoutFiles(t, []database.ChatMessage{ // Step 1 {Role: database.ChatMessageRoleAssistant, Visibility: database.ChatMessageVisibilityBoth, Content: step1Assistant}, {Role: database.ChatMessageRoleTool, Visibility: database.ChatMessageVisibilityBoth, Content: resultA}, @@ -419,7 +430,6 @@ func TestInjectMissingToolUses_DropsProviderExecutedOrphans(t *testing.T) { fantasy.TextContent{Text: "?"}, })}, }) - require.NoError(t, err) // Expected message sequence: // [0] assistant [tool_use A, B, C(PE)] @@ -492,13 +502,12 @@ func TestInjectMissingToolUses_DropsOnlyProviderExecutedMessage(t *testing.T) { false, false, true, ) - prompt, err := chatprompt.ConvertMessages([]database.ChatMessage{ + prompt := convertMessagesWithoutFiles(t, []database.ChatMessage{ {Role: database.ChatMessageRoleAssistant, Visibility: database.ChatMessageVisibilityBoth, Content: assistantContent}, {Role: database.ChatMessageRoleTool, Visibility: database.ChatMessageVisibilityBoth, Content: localResult}, {Role: database.ChatMessageRoleAssistant, Visibility: database.ChatMessageVisibilityBoth, Content: assistant2Content}, {Role: database.ChatMessageRoleTool, Visibility: database.ChatMessageVisibilityBoth, Content: peResult}, }) - require.NoError(t, err) // The PE-only tool message should be dropped entirely. // Expected: assistant, tool(local), assistant(text) @@ -537,13 +546,12 @@ func TestProviderExecutedResultInAssistantContent(t *testing.T) { fantasy.TextContent{Text: "Here is what I found."}, }) - prompt, err := chatprompt.ConvertMessages([]database.ChatMessage{ + prompt := convertMessagesWithoutFiles(t, []database.ChatMessage{ {Role: database.ChatMessageRoleAssistant, Visibility: database.ChatMessageVisibilityBoth, Content: assistantContent}, {Role: database.ChatMessageRoleUser, Visibility: database.ChatMessageVisibilityBoth, Content: mustMarshalContent(t, []fantasy.Content{ fantasy.TextContent{Text: "Thanks!"}, })}, }) - require.NoError(t, err) // Should be 2 messages: assistant + user. require.Len(t, prompt, 2) @@ -610,7 +618,7 @@ func TestProviderExecutedResult_LegacyToolRow(t *testing.T) { false, false, false, ) - prompt, err := chatprompt.ConvertMessages([]database.ChatMessage{ + prompt := convertMessagesWithoutFiles(t, []database.ChatMessage{ {Role: database.ChatMessageRoleAssistant, Visibility: database.ChatMessageVisibilityBoth, Content: assistantContent}, {Role: database.ChatMessageRoleTool, Visibility: database.ChatMessageVisibilityBoth, Content: peResult}, {Role: database.ChatMessageRoleTool, Visibility: database.ChatMessageVisibilityBoth, Content: execResult}, @@ -618,7 +626,6 @@ func TestProviderExecutedResult_LegacyToolRow(t *testing.T) { fantasy.TextContent{Text: "next"}, })}, }) - require.NoError(t, err) // The PE tool result should be dropped by injectMissingToolUses, // leaving: assistant, tool(exec), user. @@ -1902,6 +1909,79 @@ func TestConvertMessagesWithFiles_IsSyntheticPaste(t *testing.T) { } } +func TestConvertMessagesWithFiles_AssistantAttachmentIsNotReplayed(t *testing.T) { + t.Parallel() + + userFileID := uuid.New() + assistantFileID := uuid.New() + + userContent, err := chatprompt.MarshalParts([]codersdk.ChatMessagePart{ + codersdk.ChatMessageFile(userFileID, "image/png", "user.png"), + }) + require.NoError(t, err) + + assistantContent, err := chatprompt.MarshalParts([]codersdk.ChatMessagePart{ + codersdk.ChatMessageText("I attached logs above."), + codersdk.ChatMessageFile(assistantFileID, "text/plain", "agent.log"), + }) + require.NoError(t, err) + + var resolverCalls [][]uuid.UUID + resolver := func(_ context.Context, ids []uuid.UUID) (map[uuid.UUID]chatprompt.FileData, error) { + resolverCalls = append(resolverCalls, append([]uuid.UUID(nil), ids...)) + result := make(map[uuid.UUID]chatprompt.FileData, len(ids)) + for _, id := range ids { + switch id { + case userFileID: + result[id] = chatprompt.FileData{ + Name: "user.png", + Data: []byte("png-bytes"), + MediaType: "image/png", + } + case assistantFileID: + t.Fatalf("assistant attachment should not be resolved for prompt replay") + } + } + return result, nil + } + + prompt, err := chatprompt.ConvertMessagesWithFiles( + context.Background(), + []database.ChatMessage{ + { + Role: database.ChatMessageRoleUser, + Visibility: database.ChatMessageVisibilityBoth, + Content: userContent, + }, + { + Role: database.ChatMessageRoleAssistant, + Visibility: database.ChatMessageVisibilityBoth, + Content: assistantContent, + }, + }, + resolver, + slogtest.Make(t, nil), + ) + require.NoError(t, err) + require.Len(t, resolverCalls, 1) + require.Equal(t, []uuid.UUID{userFileID}, resolverCalls[0]) + require.Len(t, prompt, 2) + + userFilePart, ok := fantasy.AsMessagePart[fantasy.FilePart](prompt[0].Content[0]) + require.True(t, ok, "expected resolved user file to stay in the prompt") + require.Equal(t, []byte("png-bytes"), userFilePart.Data) + require.Equal(t, "image/png", userFilePart.MediaType) + + require.Equal(t, fantasy.MessageRoleAssistant, prompt[1].Role) + require.Len(t, prompt[1].Content, 1) + assistantText, ok := fantasy.AsMessagePart[fantasy.TextPart](prompt[1].Content[0]) + require.True(t, ok, "expected assistant text to remain after attachment omission") + require.Equal(t, "I attached logs above.", assistantText.Text) + + _, hasAssistantFilePart := fantasy.AsMessagePart[fantasy.FilePart](prompt[1].Content[0]) + require.False(t, hasAssistantFilePart, "assistant attachments should not be replayed into the prompt") +} + func convertSingleResolvedFileMessage(t *testing.T, fileID uuid.UUID, fileData chatprompt.FileData) []fantasy.Message { t.Helper() diff --git a/coderd/x/chatd/chattool/attachfile.go b/coderd/x/chatd/chattool/attachfile.go new file mode 100644 index 0000000000..ee46ad8a91 --- /dev/null +++ b/coderd/x/chatd/chattool/attachfile.go @@ -0,0 +1,78 @@ +package chattool + +import ( + "context" + "strings" + + "charm.land/fantasy" + + "github.com/coder/coder/v2/codersdk/workspacesdk" +) + +// AttachFileOptions configures the attach_file tool. +type AttachFileOptions struct { + GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error) + StoreFile StoreFileFunc +} + +// AttachFileArgs are the arguments for the attach_file tool. +type AttachFileArgs struct { + Path string `json:"path"` + Name string `json:"name,omitempty"` +} + +// AttachFile returns a tool that stores a workspace file as a durable chat +// attachment so the user can download it directly from the conversation. +func AttachFile(options AttachFileOptions) fantasy.AgentTool { + return fantasy.NewAgentTool( + "attach_file", + "Attach a workspace file to the current chat so the user can download it directly from the conversation. "+ + "Use this when the user should receive an artifact such as a screenshot, log, patch, or document. "+ + "Pass an absolute file path. The file must already exist in the workspace.", + func(ctx context.Context, args AttachFileArgs, _ fantasy.ToolCall) (fantasy.ToolResponse, error) { + if options.GetWorkspaceConn == nil { + return fantasy.NewTextErrorResponse("workspace connection resolver is not configured"), nil + } + if options.StoreFile == nil { + return fantasy.NewTextErrorResponse("file storage is not configured"), nil + } + conn, err := options.GetWorkspaceConn(ctx) + if err != nil { + return fantasy.NewTextErrorResponse(err.Error()), nil + } + return executeAttachFileTool(ctx, conn, args, options.StoreFile) + }, + ) +} + +func executeAttachFileTool( + ctx context.Context, + conn workspacesdk.AgentConn, + args AttachFileArgs, + storeFile StoreFileFunc, +) (fantasy.ToolResponse, error) { + path := strings.TrimSpace(args.Path) + if path == "" { + return fantasy.NewTextErrorResponse("path is required (use an absolute path, e.g. /home/coder/build.log)"), nil + } + + attachment, size, err := storeWorkspaceAttachment( + ctx, + conn, + path, + strings.TrimSpace(args.Name), + storeFile, + ) + if err != nil { + return fantasy.NewTextErrorResponse(err.Error()), nil + } + + return WithAttachments(toolResponse(map[string]any{ + "ok": true, + "path": path, + "file_id": attachment.FileID.String(), + "name": attachment.Name, + "media_type": attachment.MediaType, + "size": size, + }), attachment), nil +} diff --git a/coderd/x/chatd/chattool/attachfile_test.go b/coderd/x/chatd/chattool/attachfile_test.go new file mode 100644 index 0000000000..4230b42459 --- /dev/null +++ b/coderd/x/chatd/chattool/attachfile_test.go @@ -0,0 +1,290 @@ +package chattool_test + +import ( + "context" + "encoding/json" + "io" + "strings" + "testing" + + "charm.land/fantasy" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk/workspacesdk" + "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" +) + +type attachFileResponse struct { + OK bool `json:"ok"` + Path string `json:"path"` + FileID string `json:"file_id"` + Name string `json:"name"` + MediaType string `json:"media_type"` + Size int `json:"size"` +} + +func TestAttachFile(t *testing.T) { + t.Parallel() + + t.Run("EmptyPathReturnsError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + tool := newAttachFileTool(t, mockConn, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + return chattool.AttachmentMetadata{}, nil + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", Name: "attach_file", Input: `{"path":""}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "path is required") + }) + + t.Run("RelativePathErrorComesFromAgent", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + ReadFile(gomock.Any(), "notes.txt", int64(0), int64(10<<20+1)). + Return(nil, "", xerrors.New(`file path must be absolute: "notes.txt"`)) + tool := newAttachFileTool(t, mockConn, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + return chattool.AttachmentMetadata{}, nil + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", Name: "attach_file", Input: `{"path":"notes.txt"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, `file path must be absolute: "notes.txt"`) + }) + + t.Run("ValidTextFileStoresAttachment", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + content := "build succeeded\n" + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/build.log", int64(0), int64(10<<20+1)). + Return(io.NopCloser(strings.NewReader(content)), "text/plain", nil) + + var storedName string + var storedType string + var storedData []byte + tool := newAttachFileTool(t, mockConn, func(_ context.Context, name string, detectName string, data []byte) (chattool.AttachmentMetadata, error) { + storedName = name + require.Equal(t, "/home/coder/build.log", detectName) + storedType = "text/plain" + storedData = append([]byte(nil), data...) + return chattool.AttachmentMetadata{ + FileID: uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"), + MediaType: storedType, + Name: name, + }, nil + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", Name: "attach_file", Input: `{"path":"/home/coder/build.log"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Equal(t, "build.log", storedName) + assert.Equal(t, "text/plain", storedType) + assert.Equal(t, []byte(content), storedData) + + decoded := decodeAttachFileResponse(t, resp) + assert.True(t, decoded.OK) + assert.Equal(t, "/home/coder/build.log", decoded.Path) + assert.Equal(t, "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", decoded.FileID) + assert.Equal(t, "build.log", decoded.Name) + assert.Equal(t, "text/plain", decoded.MediaType) + assert.Equal(t, len(content), decoded.Size) + + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + require.Len(t, attachments, 1) + assert.Equal(t, uuid.MustParse(decoded.FileID), attachments[0].FileID) + assert.Equal(t, decoded.MediaType, attachments[0].MediaType) + assert.Equal(t, decoded.Name, attachments[0].Name) + }) + + t.Run("WindowsAbsolutePathUsesBaseName", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + content := "build succeeded\n" + path := `C:\Users\coder\build.log` + mockConn.EXPECT(). + ReadFile(gomock.Any(), path, int64(0), int64(10<<20+1)). + Return(io.NopCloser(strings.NewReader(content)), "text/plain", nil) + + var storedName string + tool := newAttachFileTool(t, mockConn, func(_ context.Context, name string, detectName string, data []byte) (chattool.AttachmentMetadata, error) { + storedName = name + require.Equal(t, path, detectName) + assert.Equal(t, []byte(content), data) + return chattool.AttachmentMetadata{ + FileID: uuid.MustParse("dddddddd-eeee-ffff-0000-111111111111"), + MediaType: "text/plain", + Name: name, + }, nil + }) + input, err := json.Marshal(chattool.AttachFileArgs{Path: path}) + require.NoError(t, err) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-windows", + Name: "attach_file", + Input: string(input), + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Equal(t, "build.log", storedName) + + decoded := decodeAttachFileResponse(t, resp) + assert.Equal(t, path, decoded.Path) + assert.Equal(t, "build.log", decoded.Name) + assert.Equal(t, len(content), decoded.Size) + }) + + t.Run("CustomNameOverridePreservesJSONSubtype", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + content := `{"ok":true}` + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/report.json", int64(0), int64(10<<20+1)). + Return(io.NopCloser(strings.NewReader(content)), "text/plain", nil) + + var storedName string + var storedType string + tool := newAttachFileTool(t, mockConn, func(_ context.Context, name string, detectName string, data []byte) (chattool.AttachmentMetadata, error) { + storedName = name + require.Equal(t, "/home/coder/report.json", detectName) + storedType = "application/json" + assert.Equal(t, []byte(content), data) + return chattool.AttachmentMetadata{ + FileID: uuid.MustParse("bbbbbbbb-cccc-dddd-eeee-ffffffffffff"), + MediaType: storedType, + Name: name, + }, nil + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-json", Name: "attach_file", Input: `{"path":"/home/coder/report.json","name":"payload.txt"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Equal(t, "payload.txt", storedName) + assert.Equal(t, "application/json", storedType) + + decoded := decodeAttachFileResponse(t, resp) + assert.Equal(t, "payload.txt", decoded.Name) + assert.Equal(t, "application/json", decoded.MediaType) + assert.Equal(t, len(content), decoded.Size) + }) + + t.Run("EmptyFileRejected", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/empty.txt", int64(0), int64(10<<20+1)). + Return(io.NopCloser(strings.NewReader("")), "text/plain", nil) + + tool := newAttachFileTool(t, mockConn, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + t.Fatal("storeFile should not be called for empty attachments") + return chattool.AttachmentMetadata{}, nil + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-empty", Name: "attach_file", Input: `{"path":"/home/coder/empty.txt"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "attachment is empty") + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + assert.Empty(t, attachments) + }) + + t.Run("OversizedFileRejected", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + largeContent := strings.Repeat("x", 10<<20+1) + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/build.log", int64(0), int64(10<<20+1)). + Return(io.NopCloser(strings.NewReader(largeContent)), "text/plain", nil) + + tool := newAttachFileTool(t, mockConn, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + return chattool.AttachmentMetadata{}, xerrors.New("should not be called") + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", Name: "attach_file", Input: `{"path":"/home/coder/build.log"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "attachment exceeds 10 MiB size limit") + }) + + t.Run("ReadFileError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/build.log", int64(0), int64(10<<20+1)). + Return(nil, "", xerrors.New("file not found")) + + tool := newAttachFileTool(t, mockConn, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + return chattool.AttachmentMetadata{}, nil + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", Name: "attach_file", Input: `{"path":"/home/coder/build.log"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "file not found") + }) + + t.Run("StoreFileErrorSurfaces", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/build.log", int64(0), int64(10<<20+1)). + Return(io.NopCloser(strings.NewReader("build succeeded\n")), "text/plain", nil) + + tool := newAttachFileTool(t, mockConn, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + return chattool.AttachmentMetadata{}, xerrors.New("chat already has the maximum of 20 linked files") + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-cap", Name: "attach_file", Input: `{"path":"/home/coder/build.log"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "chat already has the maximum of 20 linked files") + }) +} + +func newAttachFileTool( + t *testing.T, + mockConn *agentconnmock.MockAgentConn, + storeFile chattool.StoreFileFunc, +) fantasy.AgentTool { + t.Helper() + return chattool.AttachFile(chattool.AttachFileOptions{ + GetWorkspaceConn: func(_ context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + StoreFile: storeFile, + }) +} + +func decodeAttachFileResponse(t *testing.T, resp fantasy.ToolResponse) attachFileResponse { + t.Helper() + var result attachFileResponse + require.NoError(t, json.Unmarshal([]byte(resp.Content), &result)) + return result +} diff --git a/coderd/x/chatd/chattool/attachment.go b/coderd/x/chatd/chattool/attachment.go new file mode 100644 index 0000000000..e07fee314f --- /dev/null +++ b/coderd/x/chatd/chattool/attachment.go @@ -0,0 +1,171 @@ +package chattool + +import ( + "context" + "encoding/base64" + "encoding/json" + "io" + "strings" + + "charm.land/fantasy" + "github.com/google/uuid" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/codersdk/workspacesdk" +) + +const maxAttachmentSize = 10 << 20 // 10 MiB + +// StoreFileFunc persists a chat attachment after classifying it for durable +// storage and returns the stored attachment metadata. +type StoreFileFunc func(ctx context.Context, name string, detectName string, data []byte) (AttachmentMetadata, error) + +// AttachmentMetadata identifies a durable chat attachment that should be +// promoted into a standard file message part for the user. +type AttachmentMetadata struct { + FileID uuid.UUID `json:"file_id"` + MediaType string `json:"media_type"` + Name string `json:"name,omitempty"` +} + +type attachmentResponseMetadata struct { + Attachments []AttachmentMetadata `json:"attachments,omitempty"` +} + +func storeAttachmentData( + ctx context.Context, + storeFile StoreFileFunc, + name string, + detectName string, + data []byte, +) (AttachmentMetadata, error) { + if storeFile == nil { + return AttachmentMetadata{}, xerrors.New("file storage is not configured") + } + if len(data) == 0 { + return AttachmentMetadata{}, xerrors.New("attachment is empty") + } + if len(data) > maxAttachmentSize { + return AttachmentMetadata{}, xerrors.Errorf("attachment exceeds %d MiB size limit", maxAttachmentSize>>20) + } + + name = strings.TrimSpace(name) + if name == "" { + return AttachmentMetadata{}, xerrors.New("attachment name is required") + } + if strings.TrimSpace(detectName) == "" { + detectName = name + } + + attachment, err := storeFile(ctx, name, detectName, data) + if err != nil { + return AttachmentMetadata{}, err + } + if attachment.FileID == uuid.Nil { + return AttachmentMetadata{}, xerrors.New("stored attachment is missing file ID") + } + if attachment.MediaType == "" { + return AttachmentMetadata{}, xerrors.New("stored attachment is missing media type") + } + if attachment.Name == "" { + attachment.Name = name + } + return attachment, nil +} + +func storeWorkspaceAttachment( + ctx context.Context, + conn workspacesdk.AgentConn, + path string, + name string, + storeFile StoreFileFunc, +) (AttachmentMetadata, int, error) { + if conn == nil { + return AttachmentMetadata{}, 0, xerrors.New("workspace connection is not configured") + } + if strings.TrimSpace(path) == "" { + return AttachmentMetadata{}, 0, xerrors.New("path is required") + } + reader, _, err := conn.ReadFile(ctx, path, 0, maxAttachmentSize+1) + if err != nil { + return AttachmentMetadata{}, 0, err + } + defer reader.Close() + + data, err := io.ReadAll(io.LimitReader(reader, maxAttachmentSize+1)) + if err != nil { + return AttachmentMetadata{}, 0, err + } + if strings.TrimSpace(name) == "" { + path = strings.TrimRight(path, "/\\") + if idx := strings.LastIndexAny(path, "/\\"); idx >= 0 { + name = path[idx+1:] + } else { + name = path + } + } + attachment, err := storeAttachmentData(ctx, storeFile, name, path, data) + if err != nil { + return AttachmentMetadata{}, 0, err + } + return attachment, len(data), nil +} + +func storeScreenshotAttachment( + ctx context.Context, + storeFile StoreFileFunc, + name string, + encodedPNG string, +) (AttachmentMetadata, error) { + if strings.TrimSpace(encodedPNG) == "" { + return AttachmentMetadata{}, xerrors.New("screenshot data is empty") + } + decoder := base64.NewDecoder(base64.StdEncoding, strings.NewReader(encodedPNG)) + data, err := io.ReadAll(io.LimitReader(decoder, maxAttachmentSize+1)) + if err != nil { + return AttachmentMetadata{}, xerrors.Errorf("decode screenshot: %w", err) + } + if strings.TrimSpace(name) == "" { + name = "screenshot.png" + } + return storeAttachmentData(ctx, storeFile, name, name, data) +} + +// WithAttachments stores durable attachment metadata on a tool response so the +// persistence layer can promote the files into assistant chat attachments. +func WithAttachments( + response fantasy.ToolResponse, + attachments ...AttachmentMetadata, +) fantasy.ToolResponse { + if len(attachments) == 0 { + return response + } + return fantasy.WithResponseMetadata(response, attachmentResponseMetadata{ + Attachments: attachments, + }) +} + +// AttachmentsFromMetadata decodes durable attachment metadata from a tool +// response so the persistence layer can promote them into assistant file parts. +func AttachmentsFromMetadata(metadata string) ([]AttachmentMetadata, error) { + if strings.TrimSpace(metadata) == "" { + return nil, nil + } + + var decoded attachmentResponseMetadata + if err := json.Unmarshal([]byte(metadata), &decoded); err != nil { + return nil, xerrors.Errorf("unmarshal attachment metadata: %w", err) + } + + attachments := make([]AttachmentMetadata, 0, len(decoded.Attachments)) + for i, attachment := range decoded.Attachments { + if attachment.FileID == uuid.Nil { + return nil, xerrors.Errorf("attachment %d is missing file_id", i) + } + if attachment.MediaType == "" { + return nil, xerrors.Errorf("attachment %d is missing media_type", i) + } + attachments = append(attachments, attachment) + } + return attachments, nil +} diff --git a/coderd/x/chatd/chattool/computeruse.go b/coderd/x/chatd/chattool/computeruse.go index 6c260ebeed..7f65d44fb9 100644 --- a/coderd/x/chatd/chattool/computeruse.go +++ b/coderd/x/chatd/chattool/computeruse.go @@ -8,6 +8,7 @@ import ( "charm.land/fantasy" fantasyanthropic "charm.land/fantasy/providers/anthropic" + "cdr.dev/slog/v3" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/quartz" ) @@ -27,8 +28,10 @@ type computerUseTool struct { declaredWidth int declaredHeight int getWorkspaceConn func(ctx context.Context) (workspacesdk.AgentConn, error) + storeFile StoreFileFunc providerOptions fantasy.ProviderOptions clock quartz.Clock + logger slog.Logger } // NewComputerUseTool creates a computer use AgentTool that delegates to the @@ -38,22 +41,28 @@ type computerUseTool struct { func NewComputerUseTool( declaredWidth, declaredHeight int, getWorkspaceConn func(ctx context.Context) (workspacesdk.AgentConn, error), + storeFile StoreFileFunc, clock quartz.Clock, + logger slog.Logger, ) fantasy.AgentTool { return &computerUseTool{ declaredWidth: declaredWidth, declaredHeight: declaredHeight, getWorkspaceConn: getWorkspaceConn, + storeFile: storeFile, clock: clock, + logger: logger, } } func (*computerUseTool) Info() fantasy.ToolInfo { return fantasy.ToolInfo{ - Name: "computer", - Description: "Control the desktop: take screenshots, move the mouse, click, type, and scroll.", - Parameters: map[string]any{}, - Required: []string{}, + Name: "computer", + Description: "Control the desktop: take screenshots, move the mouse, click, type, and scroll. " + + "Use an explicit screenshot action when you want to share a screenshot with the user; " + + "those screenshots are also attached to the chat.", + Parameters: map[string]any{}, + Required: []string{}, } } @@ -110,38 +119,12 @@ func (t *computerUseTool) Run(ctx context.Context, call fantasy.ToolCall) (fanta case <-ctx.Done(): case <-timer.C: } - screenshotAction := workspacesdk.DesktopAction{ - Action: "screenshot", - ScaledWidth: &declaredWidth, - ScaledHeight: &declaredHeight, - } - screenResp, sErr := conn.ExecuteDesktopAction(ctx, screenshotAction) - if sErr != nil { - return fantasy.NewTextErrorResponse( - fmt.Sprintf("screenshot failed: %v", sErr), - ), nil - } - return fantasy.NewImageResponse( - []byte(screenResp.ScreenshotData), "image/png", - ), nil + return t.captureScreenshot(ctx, conn, declaredWidth, declaredHeight) } // For screenshot action, use ExecuteDesktopAction. if input.Action == fantasyanthropic.ActionScreenshot { - screenshotAction := workspacesdk.DesktopAction{ - Action: "screenshot", - ScaledWidth: &declaredWidth, - ScaledHeight: &declaredHeight, - } - screenResp, sErr := conn.ExecuteDesktopAction(ctx, screenshotAction) - if sErr != nil { - return fantasy.NewTextErrorResponse( - fmt.Sprintf("screenshot failed: %v", sErr), - ), nil - } - return fantasy.NewImageResponse( - []byte(screenResp.ScreenshotData), "image/png", - ), nil + return t.captureSharedScreenshot(ctx, conn, declaredWidth, declaredHeight) } // Build the action request. @@ -182,21 +165,72 @@ func (t *computerUseTool) Run(ctx context.Context, call fantasy.ToolCall) (fanta } // Take a screenshot after every action (Anthropic pattern). + return t.captureScreenshot(ctx, conn, declaredWidth, declaredHeight) +} + +func (*computerUseTool) captureScreenshot( + ctx context.Context, + conn workspacesdk.AgentConn, + declaredWidth, declaredHeight int, +) (fantasy.ToolResponse, error) { + screenResp, err := executeScreenshotAction(ctx, conn, declaredWidth, declaredHeight) + if err != nil { + return fantasy.NewTextErrorResponse( + fmt.Sprintf("screenshot failed: %v", err), + ), nil + } + return fantasy.NewImageResponse([]byte(screenResp.ScreenshotData), "image/png"), nil +} + +func (t *computerUseTool) captureSharedScreenshot( + ctx context.Context, + conn workspacesdk.AgentConn, + declaredWidth, declaredHeight int, +) (fantasy.ToolResponse, error) { + screenResp, err := executeScreenshotAction(ctx, conn, declaredWidth, declaredHeight) + if err != nil { + return fantasy.NewTextErrorResponse( + fmt.Sprintf("screenshot failed: %v", err), + ), nil + } + + attachmentName := fmt.Sprintf( + "screenshot-%s.png", + t.clock.Now().UTC().Format("2006-01-02T15-04-05Z"), + ) + if t.storeFile == nil { + t.logger.Warn(ctx, "screenshot attachment storage is not configured") + return fantasy.NewImageResponse([]byte(screenResp.ScreenshotData), "image/png"), nil + } + + attachment, err := storeScreenshotAttachment( + ctx, + t.storeFile, + attachmentName, + screenResp.ScreenshotData, + ) + response := fantasy.NewImageResponse([]byte(screenResp.ScreenshotData), "image/png") + if err != nil { + t.logger.Warn(ctx, "failed to persist screenshot attachment", + slog.F("attachment_name", attachmentName), + slog.Error(err), + ) + return response, nil + } + return WithAttachments(response, attachment), nil +} + +func executeScreenshotAction( + ctx context.Context, + conn workspacesdk.AgentConn, + declaredWidth, declaredHeight int, +) (workspacesdk.DesktopActionResponse, error) { screenshotAction := workspacesdk.DesktopAction{ Action: "screenshot", ScaledWidth: &declaredWidth, ScaledHeight: &declaredHeight, } - screenResp, sErr := conn.ExecuteDesktopAction(ctx, screenshotAction) - if sErr != nil { - return fantasy.NewTextErrorResponse( - fmt.Sprintf("screenshot failed: %v", sErr), - ), nil - } - - return fantasy.NewImageResponse( - []byte(screenResp.ScreenshotData), "image/png", - ), nil + return conn.ExecuteDesktopAction(ctx, screenshotAction) } func (t *computerUseTool) declaredActionDimensions() (declaredWidth, declaredHeight int) { @@ -206,10 +240,3 @@ func (t *computerUseTool) declaredActionDimensions() (declaredWidth, declaredHei } return t.declaredWidth, t.declaredHeight } - -// computeScaledScreenshotSize preserves the historical helper name while using -// the shared declared-geometry selection logic. -func computeScaledScreenshotSize(width, height int) (scaledWidth int, scaledHeight int) { - geometry := workspacesdk.NewDesktopGeometry(width, height) - return geometry.DeclaredWidth, geometry.DeclaredHeight -} diff --git a/coderd/x/chatd/chattool/computeruse_internal_test.go b/coderd/x/chatd/chattool/computeruse_internal_test.go deleted file mode 100644 index f773306fd5..0000000000 --- a/coderd/x/chatd/chattool/computeruse_internal_test.go +++ /dev/null @@ -1,85 +0,0 @@ -package chattool - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestComputeScaledScreenshotSize(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - width, height int - wantW, wantH int - }{ - { - name: "1920x1080_prefers_standard_1280x720", - width: 1920, - height: 1080, - wantW: 1280, - wantH: 720, - }, - { - name: "1280x800_no_scaling", - width: 1280, - height: 800, - wantW: 1280, - wantH: 800, - }, - { - name: "3840x2160_prefers_standard_1280x720", - width: 3840, - height: 2160, - wantW: 1280, - wantH: 720, - }, - { - name: "1568x1000_prefers_standard_1280x816", - width: 1568, - height: 1000, - wantW: 1280, - wantH: 816, - }, - { - name: "100x100_small_display", - width: 100, - height: 100, - wantW: 100, - wantH: 100, - }, - { - name: "4000x3000_prefers_standard_1024x768", - width: 4000, - height: 3000, - wantW: 1024, - wantH: 768, - }, - { - name: "1920x1200_prefers_standard_1280x800", - width: 1920, - height: 1200, - wantW: 1280, - wantH: 800, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - gotW, gotH := computeScaledScreenshotSize(tt.width, tt.height) - assert.Equal(t, tt.wantW, gotW) - assert.Equal(t, tt.wantH, gotH) - - // Invariant: results must respect Anthropic constraints. - const maxLongEdge = 1568 - const maxTotalPixels = 1_150_000 - longEdge := max(gotW, gotH) - assert.LessOrEqual(t, longEdge, maxLongEdge, - "long edge %d exceeds max %d", longEdge, maxLongEdge) - assert.LessOrEqual(t, gotW*gotH, maxTotalPixels, - "total pixels %d exceeds max %d", gotW*gotH, maxTotalPixels) - }) - } -} diff --git a/coderd/x/chatd/chattool/computeruse_test.go b/coderd/x/chatd/chattool/computeruse_test.go index 50c4dc0d57..4ba5308d2d 100644 --- a/coderd/x/chatd/chattool/computeruse_test.go +++ b/coderd/x/chatd/chattool/computeruse_test.go @@ -1,31 +1,25 @@ package chattool_test import ( + "bytes" "context" + "encoding/base64" "testing" "charm.land/fantasy" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "golang.org/x/xerrors" + "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/x/chatd/chattool" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" "github.com/coder/quartz" ) -func TestComputerUseTool_Info(t *testing.T) { - t.Parallel() - - geometry := workspacesdk.DefaultDesktopGeometry() - tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, nil, quartz.NewReal()) - info := tool.Info() - assert.Equal(t, "computer", info.Name) - assert.NotEmpty(t, info.Description) -} - func TestComputerUseProviderTool(t *testing.T) { t.Parallel() @@ -39,17 +33,6 @@ func TestComputerUseProviderTool(t *testing.T) { assert.Equal(t, int64(geometry.DeclaredHeight), pdt.Args["display_height_px"]) } -func TestComputerUseProviderTool_PrefersDeclaredGeometry(t *testing.T) { - t.Parallel() - - geometry := workspacesdk.NewDesktopGeometry(1920, 1080) - def := chattool.ComputerUseProviderTool(geometry.DeclaredWidth, geometry.DeclaredHeight) - pdt, ok := def.(fantasy.ProviderDefinedTool) - require.True(t, ok, "ComputerUseProviderTool should return a ProviderDefinedTool") - assert.Equal(t, int64(1280), pdt.Args["display_width_px"]) - assert.Equal(t, int64(720), pdt.Args["display_height_px"]) -} - func TestComputerUseTool_Run_Screenshot(t *testing.T) { t.Parallel() @@ -67,7 +50,7 @@ func TestComputerUseTool_Run_Screenshot(t *testing.T) { assert.Equal(t, geometry.DeclaredHeight, *action.ScaledHeight) return workspacesdk.DesktopActionResponse{ Output: "screenshot", - ScreenshotData: "base64png", + ScreenshotData: "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4n539HwAHFwLVF8kc1wAAAABJRU5ErkJggg==", ScreenshotWidth: geometry.DeclaredWidth, ScreenshotHeight: geometry.DeclaredHeight, }, nil @@ -75,7 +58,7 @@ func TestComputerUseTool_Run_Screenshot(t *testing.T) { tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { return mockConn, nil - }, quartz.NewReal()) + }, nil, quartz.NewReal(), slogtest.Make(t, nil)) call := fantasy.ToolCall{ ID: "test-1", @@ -87,16 +70,149 @@ func TestComputerUseTool_Run_Screenshot(t *testing.T) { require.NoError(t, err) assert.Equal(t, "image", resp.Type) assert.Equal(t, "image/png", resp.MediaType) - assert.Equal(t, []byte("base64png"), resp.Data) + assert.Equal(t, []byte("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4n539HwAHFwLVF8kc1wAAAABJRU5ErkJggg=="), resp.Data) assert.False(t, resp.IsError) } +func TestComputerUseTool_Run_Screenshot_PersistsAttachment(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + geometry := workspacesdk.DefaultDesktopGeometry() + const screenshotPNG = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4n539HwAHFwLVF8kc1wAAAABJRU5ErkJggg==" + + mockConn.EXPECT().ExecuteDesktopAction( + gomock.Any(), + gomock.AssignableToTypeOf(workspacesdk.DesktopAction{}), + ).DoAndReturn(func(_ context.Context, action workspacesdk.DesktopAction) (workspacesdk.DesktopActionResponse, error) { + require.Equal(t, "screenshot", action.Action) + return workspacesdk.DesktopActionResponse{ + Output: "screenshot", + ScreenshotData: screenshotPNG, + ScreenshotWidth: geometry.DeclaredWidth, + ScreenshotHeight: geometry.DeclaredHeight, + }, nil + }) + + var storedName string + var storedType string + var storedData []byte + tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, func(_ context.Context, name string, detectName string, data []byte) (chattool.AttachmentMetadata, error) { + storedName = name + require.Equal(t, name, detectName) + storedType = "image/png" + storedData = append([]byte(nil), data...) + return chattool.AttachmentMetadata{ + FileID: uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"), + MediaType: storedType, + Name: name, + }, nil + }, quartz.NewReal(), slogtest.Make(t, nil)) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "test-screenshot-persist", Name: "computer", Input: `{"action":"screenshot"}`, + }) + require.NoError(t, err) + assert.Equal(t, "image", resp.Type) + assert.Equal(t, "image/png", resp.MediaType) + assert.Equal(t, []byte(screenshotPNG), resp.Data) + assert.Contains(t, storedName, "screenshot-") + assert.Equal(t, "image/png", storedType) + expectedPNG, decodeErr := base64.StdEncoding.DecodeString(screenshotPNG) + require.NoError(t, decodeErr) + require.Equal(t, expectedPNG, storedData) + + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + require.Len(t, attachments, 1) + assert.Equal(t, uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"), attachments[0].FileID) + assert.Equal(t, "image/png", attachments[0].MediaType) +} + +func TestComputerUseTool_Run_Screenshot_StoreErrorFallsBackToImage(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + geometry := workspacesdk.DefaultDesktopGeometry() + const screenshotPNG = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4n539HwAHFwLVF8kc1wAAAABJRU5ErkJggg==" + + mockConn.EXPECT().ExecuteDesktopAction( + gomock.Any(), + gomock.AssignableToTypeOf(workspacesdk.DesktopAction{}), + ).Return(workspacesdk.DesktopActionResponse{ + Output: "screenshot", + ScreenshotData: screenshotPNG, + ScreenshotWidth: geometry.DeclaredWidth, + ScreenshotHeight: geometry.DeclaredHeight, + }, nil) + + tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + return chattool.AttachmentMetadata{}, xerrors.New("chat already has the maximum of 20 linked files") + }, quartz.NewReal(), slogtest.Make(t, nil)) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "test-screenshot-store-error", Name: "computer", Input: `{"action":"screenshot"}`, + }) + require.NoError(t, err) + assert.Equal(t, "image", resp.Type) + assert.Equal(t, "image/png", resp.MediaType) + assert.False(t, resp.IsError) + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + assert.Empty(t, attachments) +} + +func TestComputerUseTool_Run_Screenshot_OversizedAttachmentFallsBackToImage(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + geometry := workspacesdk.DefaultDesktopGeometry() + oversizedScreenshot := base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{0xAB}, 10<<20+1)) + + mockConn.EXPECT().ExecuteDesktopAction( + gomock.Any(), + gomock.AssignableToTypeOf(workspacesdk.DesktopAction{}), + ).Return(workspacesdk.DesktopActionResponse{ + Output: "screenshot", + ScreenshotData: oversizedScreenshot, + ScreenshotWidth: geometry.DeclaredWidth, + ScreenshotHeight: geometry.DeclaredHeight, + }, nil) + + tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + t.Fatal("storeFile should not be called for oversized screenshots") + return chattool.AttachmentMetadata{}, nil + }, quartz.NewReal(), slogtest.Make(t, nil)) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "test-screenshot-oversized", Name: "computer", Input: `{"action":"screenshot"}`, + }) + require.NoError(t, err) + assert.Equal(t, "image", resp.Type) + assert.Equal(t, "image/png", resp.MediaType) + assert.False(t, resp.IsError) + require.Len(t, resp.Data, len(oversizedScreenshot)) + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + assert.Empty(t, attachments) +} + func TestComputerUseTool_Run_LeftClick(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) geometry := workspacesdk.DefaultDesktopGeometry() + followUpScreenshot := base64.StdEncoding.EncodeToString([]byte("after-click")) mockConn.EXPECT().ExecuteDesktopAction( gomock.Any(), @@ -122,7 +238,7 @@ func TestComputerUseTool_Run_LeftClick(t *testing.T) { assert.Equal(t, geometry.DeclaredHeight, *action.ScaledHeight) return workspacesdk.DesktopActionResponse{ Output: "screenshot", - ScreenshotData: "after-click", + ScreenshotData: followUpScreenshot, ScreenshotWidth: geometry.DeclaredWidth, ScreenshotHeight: geometry.DeclaredHeight, }, nil @@ -130,7 +246,10 @@ func TestComputerUseTool_Run_LeftClick(t *testing.T) { tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { return mockConn, nil - }, quartz.NewReal()) + }, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + t.Fatal("storeFile should not be called for left_click follow-up screenshots") + return chattool.AttachmentMetadata{}, nil + }, quartz.NewReal(), slogtest.Make(t, nil)) call := fantasy.ToolCall{ ID: "test-2", @@ -141,7 +260,10 @@ func TestComputerUseTool_Run_LeftClick(t *testing.T) { resp, err := tool.Run(context.Background(), call) require.NoError(t, err) assert.Equal(t, "image", resp.Type) - assert.Equal(t, []byte("after-click"), resp.Data) + assert.Equal(t, []byte(followUpScreenshot), resp.Data) + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + assert.Empty(t, attachments) } func TestComputerUseTool_Run_Wait(t *testing.T) { @@ -150,6 +272,7 @@ func TestComputerUseTool_Run_Wait(t *testing.T) { ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) geometry := workspacesdk.DefaultDesktopGeometry() + followUpScreenshot := base64.StdEncoding.EncodeToString([]byte("after-wait")) mockConn.EXPECT().ExecuteDesktopAction( gomock.Any(), @@ -161,7 +284,7 @@ func TestComputerUseTool_Run_Wait(t *testing.T) { assert.Equal(t, geometry.DeclaredHeight, *action.ScaledHeight) return workspacesdk.DesktopActionResponse{ Output: "screenshot", - ScreenshotData: "after-wait", + ScreenshotData: followUpScreenshot, ScreenshotWidth: geometry.DeclaredWidth, ScreenshotHeight: geometry.DeclaredHeight, }, nil @@ -169,7 +292,10 @@ func TestComputerUseTool_Run_Wait(t *testing.T) { tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { return mockConn, nil - }, quartz.NewReal()) + }, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + t.Fatal("storeFile should not be called for wait screenshots") + return chattool.AttachmentMetadata{}, nil + }, quartz.NewReal(), slogtest.Make(t, nil)) call := fantasy.ToolCall{ ID: "test-3", @@ -181,8 +307,11 @@ func TestComputerUseTool_Run_Wait(t *testing.T) { require.NoError(t, err) assert.Equal(t, "image", resp.Type) assert.Equal(t, "image/png", resp.MediaType) - assert.Equal(t, []byte("after-wait"), resp.Data) + assert.Equal(t, []byte(followUpScreenshot), resp.Data) assert.False(t, resp.IsError) + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + assert.Empty(t, attachments) } func TestComputerUseTool_Run_ConnError(t *testing.T) { @@ -191,7 +320,7 @@ func TestComputerUseTool_Run_ConnError(t *testing.T) { geometry := workspacesdk.DefaultDesktopGeometry() tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { return nil, xerrors.New("workspace not available") - }, quartz.NewReal()) + }, nil, quartz.NewReal(), slogtest.Make(t, nil)) call := fantasy.ToolCall{ ID: "test-4", @@ -211,7 +340,7 @@ func TestComputerUseTool_Run_InvalidInput(t *testing.T) { geometry := workspacesdk.DefaultDesktopGeometry() tool := chattool.NewComputerUseTool(geometry.DeclaredWidth, geometry.DeclaredHeight, func(_ context.Context) (workspacesdk.AgentConn, error) { return nil, xerrors.New("should not be called") - }, quartz.NewReal()) + }, nil, quartz.NewReal(), slogtest.Make(t, nil)) call := fantasy.ToolCall{ ID: "test-5", diff --git a/coderd/x/chatd/chattool/proposeplan.go b/coderd/x/chatd/chattool/proposeplan.go index 2b18ee6db5..12b186d6b0 100644 --- a/coderd/x/chatd/chattool/proposeplan.go +++ b/coderd/x/chatd/chattool/proposeplan.go @@ -7,7 +7,6 @@ import ( "strings" "charm.land/fantasy" - "github.com/google/uuid" "github.com/coder/coder/v2/codersdk/workspacesdk" ) @@ -18,7 +17,7 @@ const maxProposePlanSize = 32 * 1024 // 32 KiB type ProposePlanOptions struct { GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error) ResolvePlanPath func(context.Context) (chatPath string, home string, err error) - StoreFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error) + StoreFile StoreFileFunc IsPlanTurn bool } @@ -72,7 +71,7 @@ func executeProposePlanTool( conn workspacesdk.AgentConn, args ProposePlanArgs, resolvePlanPath func(context.Context) (chatPath string, home string, err error), - storeFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error), + storeFile StoreFileFunc, ) (fantasy.ToolResponse, error) { requestedPath := strings.TrimSpace(args.Path) if requestedPath == "" { @@ -113,16 +112,16 @@ func executeProposePlanTool( return fantasy.NewTextErrorResponse("plan file exceeds 32 KiB size limit"), nil } - fileID, err := storeFile(ctx, filepath.Base(requestedPath), "text/markdown", data) + attachment, err := storeFile(ctx, filepath.Base(requestedPath), requestedPath, data) if err != nil { return fantasy.NewTextErrorResponse("failed to store plan file: " + err.Error()), nil } - return toolResponse(map[string]any{ + return WithAttachments(toolResponse(map[string]any{ "ok": true, "path": requestedPath, "kind": "plan", - "file_id": fileID.String(), - "media_type": "text/markdown", - }), nil + "file_id": attachment.FileID.String(), + "media_type": attachment.MediaType, + }), attachment), nil } diff --git a/coderd/x/chatd/chattool/proposeplan_test.go b/coderd/x/chatd/chattool/proposeplan_test.go index c6d87b5857..423d893d4a 100644 --- a/coderd/x/chatd/chattool/proposeplan_test.go +++ b/coderd/x/chatd/chattool/proposeplan_test.go @@ -4,8 +4,10 @@ import ( "context" "encoding/json" "io" + "path/filepath" "strings" "testing" + "testing/iotest" "charm.land/fantasy" "github.com/google/uuid" @@ -30,13 +32,13 @@ type proposePlanResponse struct { func TestProposePlan(t *testing.T) { t.Parallel() - t.Run("RejectsEmptyPath", func(t *testing.T) { + t.Run("EmptyPathReturnsError", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) storeFile, _ := fakeStoreFile(t) - tool := newProposePlanToolWithPlanPath(t, mockConn, storeFile, nil, false) + tool := newProposePlanTool(t, mockConn, storeFile) resp, err := tool.Run(context.Background(), fantasy.ToolCall{ ID: "call-1", Name: "propose_plan", @@ -47,23 +49,399 @@ func TestProposePlan(t *testing.T) { assert.Equal(t, "path is required (use the chat-specific absolute plan path)", resp.Content) }) - t.Run("RejectsNonMarkdownPath", func(t *testing.T) { + t.Run("WhitespaceOnlyPathReturnsError", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) storeFile, _ := fakeStoreFile(t) - tool := newProposePlanToolWithPlanPath(t, mockConn, storeFile, nil, false) + tool := newProposePlanTool(t, mockConn, storeFile) resp, err := tool.Run(context.Background(), fantasy.ToolCall{ ID: "call-1", Name: "propose_plan", - Input: `{"path":"/home/coder/.coder/plans/PLAN-chat.txt"}`, + Input: `{"path":" "}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, "path is required (use the chat-specific absolute plan path)", resp.Content) + }) + + t.Run("NonMdPathReturnsError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanTool(t, mockConn, storeFile) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/plan.txt"}`, }) require.NoError(t, err) assert.True(t, resp.IsError) assert.Equal(t, "path must end with .md", resp.Content) }) + t.Run("RelativePlanPathReturnsError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + storeFile, _ := fakeStoreFile(t) + resolvePlanPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + ) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"plan.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + assert.Equal(t, relativePlanPathMessage(), resp.Content) + }) + + t.Run("OversizedFileRejected", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + largeContent := strings.Repeat("x", 32*1024+1) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/PLAN.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader(largeContent)), "text/markdown", nil) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanTool(t, mockConn, storeFile) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/PLAN.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, "plan file exceeds 32 KiB size limit", resp.Content) + }) + + t.Run("ExactBoundaryFileSucceeds", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + content := strings.Repeat("x", 32*1024) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/PLAN.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader(content)), "text/markdown", nil) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanTool(t, mockConn, storeFile) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/PLAN.md"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + }) + + t.Run("ValidPlanReadsFile", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/docs/PLAN.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Plan\n\nContent")), "text/markdown", nil) + + storeFile, stored := fakeStoreFile(t) + planPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + planPathCalled = true + return "/home/coder/.coder/plans/PLAN-xxx.md", "/home/coder", nil + }, + ) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/docs/PLAN.md"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.True(t, planPathCalled) + + result := decodeProposePlanResponse(t, resp) + assert.True(t, result.OK) + assert.Equal(t, "/home/coder/docs/PLAN.md", result.Path) + assert.Equal(t, "plan", result.Kind) + assert.Equal(t, "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", result.FileID) + assert.Equal(t, "text/markdown", result.MediaType) + assert.Equal(t, []byte("# Plan\n\nContent"), *stored) + assert.NotContains(t, resp.Content, "content") + + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + require.Len(t, attachments, 1) + assert.Equal(t, uuid.MustParse(result.FileID), attachments[0].FileID) + assert.Equal(t, result.MediaType, attachments[0].MediaType) + assert.Equal(t, filepath.Base(result.Path), attachments[0].Name) + }) + + t.Run("NestedPlanPathUnderHomeIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/myproject/plan.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Nested Plan")), "text/markdown", nil) + + storeFile, stored := fakeStoreFile(t) + planPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + planPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + ) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/myproject/plan.md"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.True(t, planPathCalled) + + result := decodeProposePlanResponse(t, resp) + assert.True(t, result.OK) + assert.Equal(t, "/home/coder/myproject/plan.md", result.Path) + assert.Equal(t, []byte("# Nested Plan"), *stored) + }) + + t.Run("FileNotFound", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/PLAN.md", int64(0), int64(32*1024+1)). + Return(nil, "", xerrors.New("file not found")) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanTool(t, mockConn, storeFile) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/PLAN.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "file not found") + }) + + t.Run("ReadFileError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/PLAN.md", int64(0), int64(32*1024+1)). + Return(nil, "", xerrors.New("read failed")) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanTool(t, mockConn, storeFile) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/PLAN.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, "read failed", resp.Content) + }) + + t.Run("ReadAllError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/PLAN.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(iotest.ErrReader(xerrors.New("connection reset"))), "text/markdown", nil) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanTool(t, mockConn, storeFile) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/PLAN.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "connection reset") + }) + + t.Run("StoreFileError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/PLAN.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Plan")), "text/markdown", nil) + + tool := newProposePlanTool(t, mockConn, func(_ context.Context, _ string, _ string, _ []byte) (chattool.AttachmentMetadata, error) { + return chattool.AttachmentMetadata{}, xerrors.New("storage unavailable") + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/PLAN.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, "failed to store plan file: storage unavailable", resp.Content) + }) + + t.Run("RejectsSharedPlanPathWithResolvedPath", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + ) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"` + chattool.LegacySharedPlanPath + `"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal( + t, + sharedPlanPathResolvedMessage(chattool.LegacySharedPlanPath, "/home/coder/.coder/plans/PLAN-chat.md"), + resp.Content, + ) + }) + + t.Run("RejectsSharedPlanPathWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + ) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"` + chattool.LegacySharedPlanPath + `"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, planPathVerificationMessage(chattool.LegacySharedPlanPath), resp.Content) + }) + + t.Run("PerChatPlanPathIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + chatPlanPath := "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md" + + mockConn.EXPECT(). + ReadFile(gomock.Any(), chatPlanPath, int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Per-Chat Plan")), "text/markdown", nil) + + storeFile, stored := fakeStoreFile(t) + resolvePlanPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return chatPlanPath, "/home/coder", nil + }, + ) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"` + chatPlanPath + `"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + + result := decodeProposePlanResponse(t, resp) + assert.True(t, result.OK) + assert.Equal(t, chatPlanPath, result.Path) + assert.Equal(t, []byte("# Per-Chat Plan"), *stored) + }) + + t.Run("NestedPlanPathAllowedWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/myproject/plan.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Nested Plan")), "text/markdown", nil) + + storeFile, stored := fakeStoreFile(t) + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + ) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/myproject/plan.md"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + + result := decodeProposePlanResponse(t, resp) + assert.True(t, result.OK) + assert.Equal(t, "/home/coder/myproject/plan.md", result.Path) + assert.Equal(t, []byte("# Nested Plan"), *stored) + }) + t.Run("PlanTurnDefaultsEmptyPathToResolvedPath", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) @@ -126,27 +504,6 @@ func TestProposePlan(t *testing.T) { assert.True(t, resp.IsError) assert.Equal(t, "during plan turns, propose_plan path must be "+chatPlanPath, resp.Content) }) - t.Run("RejectsReadFileErrors", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - mockConn := agentconnmock.NewMockAgentConn(ctrl) - chatPlanPath := "/home/coder/.coder/plans/PLAN-chat.md" - - mockConn.EXPECT(). - ReadFile(gomock.Any(), chatPlanPath, int64(0), int64(32*1024+1)). - Return(nil, "", xerrors.New("read failed")) - - storeFile, _ := fakeStoreFile(t) - tool := newProposePlanToolWithPlanPath(t, mockConn, storeFile, nil, false) - resp, err := tool.Run(context.Background(), fantasy.ToolCall{ - ID: "call-1", - Name: "propose_plan", - Input: `{"path":"` + chatPlanPath + `"}`, - }) - require.NoError(t, err) - assert.True(t, resp.IsError) - assert.Equal(t, "read failed", resp.Content) - }) t.Run("PlanTurnRejectsEmptyPlan", func(t *testing.T) { t.Parallel() @@ -163,9 +520,9 @@ func TestProposePlan(t *testing.T) { tool := newProposePlanToolWithPlanPath( t, mockConn, - func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error) { + func(ctx context.Context, name string, detectName string, data []byte) (chattool.AttachmentMetadata, error) { storeCalled = true - return storeFile(ctx, name, mediaType, data) + return storeFile(ctx, name, detectName, data) }, func(context.Context) (string, string, error) { return chatPlanPath, "/home/coder", nil @@ -185,97 +542,106 @@ func TestProposePlan(t *testing.T) { assert.Nil(t, *stored) }) - t.Run("RejectsOversizedPlan", func(t *testing.T) { + t.Run("WorkspaceConnectionError", func(t *testing.T) { t.Parallel() - ctrl := gomock.NewController(t) - mockConn := agentconnmock.NewMockAgentConn(ctrl) - chatPlanPath := "/home/coder/.coder/plans/PLAN-chat.md" - - mockConn.EXPECT(). - ReadFile(gomock.Any(), chatPlanPath, int64(0), int64(32*1024+1)). - Return(io.NopCloser(strings.NewReader(strings.Repeat("x", 32*1024+1))), "text/markdown", nil) - - storeFile, stored := fakeStoreFile(t) - storeCalled := false - tool := newProposePlanToolWithPlanPath( - t, - mockConn, - func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error) { - storeCalled = true - return storeFile(ctx, name, mediaType, data) + storeFile, _ := fakeStoreFile(t) + tool := chattool.ProposePlan(chattool.ProposePlanOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return nil, xerrors.New("connection failed") }, - nil, - false, - ) + StoreFile: storeFile, + }) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ ID: "call-1", Name: "propose_plan", - Input: `{"path":"` + chatPlanPath + `"}`, + Input: `{"path":"/home/coder/PLAN.md"}`, }) require.NoError(t, err) assert.True(t, resp.IsError) - assert.Equal(t, "plan file exceeds 32 KiB size limit", resp.Content) - assert.False(t, storeCalled) - assert.Nil(t, *stored) + assert.Contains(t, resp.Content, "connection failed") }) - t.Run("PropagatesStoreFileErrors", func(t *testing.T) { + t.Run("NilWorkspaceResolver", func(t *testing.T) { t.Parallel() - ctrl := gomock.NewController(t) - mockConn := agentconnmock.NewMockAgentConn(ctrl) - chatPlanPath := "/home/coder/.coder/plans/PLAN-chat.md" + tool := chattool.ProposePlan(chattool.ProposePlanOptions{}) - mockConn.EXPECT(). - ReadFile(gomock.Any(), chatPlanPath, int64(0), int64(32*1024+1)). - Return(io.NopCloser(strings.NewReader("# Plan")), "text/markdown", nil) - - tool := newProposePlanToolWithPlanPath( - t, - mockConn, - func(context.Context, string, string, []byte) (uuid.UUID, error) { - return uuid.Nil, xerrors.New("store failed") - }, - nil, - false, - ) resp, err := tool.Run(context.Background(), fantasy.ToolCall{ ID: "call-1", Name: "propose_plan", - Input: `{"path":"` + chatPlanPath + `"}`, + Input: `{"path":"/home/coder/PLAN.md"}`, }) require.NoError(t, err) assert.True(t, resp.IsError) - assert.Equal(t, "failed to store plan file: store failed", resp.Content) + assert.Contains(t, resp.Content, "workspace connection resolver is not configured") }) + + t.Run("NilStoreFile", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + tool := chattool.ProposePlan(chattool.ProposePlanOptions{ + GetWorkspaceConn: func(_ context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/PLAN.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "file storage is not configured") + }) +} + +func newProposePlanTool( + t *testing.T, + mockConn *agentconnmock.MockAgentConn, + storeFile chattool.StoreFileFunc, +) fantasy.AgentTool { + t.Helper() + return newProposePlanToolWithPlanPath(t, mockConn, storeFile, nil) } func newProposePlanToolWithPlanPath( t *testing.T, mockConn *agentconnmock.MockAgentConn, - storeFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error), + storeFile chattool.StoreFileFunc, resolvePlanPath func(context.Context) (string, string, error), - isPlanTurn bool, + isPlanTurn ...bool, ) fantasy.AgentTool { t.Helper() + enabled := false + if len(isPlanTurn) > 0 { + enabled = isPlanTurn[0] + } return chattool.ProposePlan(chattool.ProposePlanOptions{ GetWorkspaceConn: func(_ context.Context) (workspacesdk.AgentConn, error) { return mockConn, nil }, ResolvePlanPath: resolvePlanPath, StoreFile: storeFile, - IsPlanTurn: isPlanTurn, + IsPlanTurn: enabled, }) } -func fakeStoreFile(t *testing.T) (func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error), *[]byte) { +func fakeStoreFile(t *testing.T) (chattool.StoreFileFunc, *[]byte) { t.Helper() var stored []byte - return func(_ context.Context, name string, mediaType string, data []byte) (uuid.UUID, error) { + return func(_ context.Context, name string, detectName string, data []byte) (chattool.AttachmentMetadata, error) { assert.NotEmpty(t, name) - assert.Equal(t, "text/markdown", mediaType) + assert.NotEmpty(t, detectName) stored = append([]byte(nil), data...) - return uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"), nil + return chattool.AttachmentMetadata{ + FileID: uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"), + MediaType: "text/markdown", + Name: name, + }, nil }, &stored } diff --git a/coderd/x/chatd/recording.go b/coderd/x/chatd/recording.go index f8b499b3f3..c9bf58922b 100644 --- a/coderd/x/chatd/recording.go +++ b/coderd/x/chatd/recording.go @@ -9,12 +9,12 @@ import ( "mime/multipart" "github.com/google/uuid" - "golang.org/x/xerrors" "cdr.dev/slog/v3" + "github.com/coder/coder/v2/coderd/chatfiles" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" - "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/coderd/x/chatd/chattool" "github.com/coder/coder/v2/codersdk/workspacesdk" ) @@ -33,24 +33,40 @@ func (p *Server) stopAndStoreRecording( ctx context.Context, conn workspacesdk.AgentConn, recordingID string, + parentChatID uuid.UUID, ownerID uuid.UUID, workspaceID uuid.NullUUID, - chatID uuid.UUID, ) recordingResult { var result recordingResult + workspaceIDValue := "" + if workspaceID.Valid { + workspaceIDValue = workspaceID.UUID.String() + } + recordingWarnFields := []slog.Field{ + slog.F("recording_id", recordingID), + slog.F("parent_chat_id", parentChatID.String()), + slog.F("workspace_id", workspaceIDValue), + } + warn := func(msg string, fields ...slog.Field) { + allFields := make([]slog.Field, 0, len(recordingWarnFields)+len(fields)) + allFields = append(allFields, recordingWarnFields...) + allFields = append(allFields, fields...) + p.logger.Warn(ctx, msg, allFields...) + } + select { case p.recordingSem <- struct{}{}: defer func() { <-p.recordingSem }() case <-ctx.Done(): - p.logger.Warn(ctx, "context canceled waiting for recording semaphore", slog.Error(ctx.Err())) + warn("context canceled waiting for recording semaphore", slog.Error(ctx.Err())) return result } resp, err := conn.StopDesktopRecording(ctx, workspacesdk.StopDesktopRecordingRequest{RecordingID: recordingID}) if err != nil { - p.logger.Warn(ctx, "failed to stop desktop recording", + warn("failed to stop desktop recording", slog.Error(err)) return result } @@ -58,29 +74,30 @@ func (p *Server) stopAndStoreRecording( _, params, err := mime.ParseMediaType(resp.ContentType) if err != nil { - p.logger.Warn(ctx, "failed to parse content type from recording response", + warn("failed to parse content type from recording response", slog.F("content_type", resp.ContentType), slog.Error(err)) return result } boundary := params["boundary"] if boundary == "" { - p.logger.Warn(ctx, "missing boundary in recording response content type", + warn("missing boundary in recording response content type", slog.F("content_type", resp.ContentType)) return result } if !workspaceID.Valid { - p.logger.Warn(ctx, "chat has no workspace, cannot store recording") + warn("chat has no workspace, cannot store recording") return result } // The chatd actor is used here because the recording is stored on // behalf of the chat system, not a specific user request. //nolint:gocritic // AsChatd is required to read the workspace for org lookup. - ws, err := p.db.GetWorkspaceByID(dbauthz.AsChatd(ctx), workspaceID.UUID) + chatdCtx := dbauthz.AsChatd(ctx) + ws, err := p.db.GetWorkspaceByID(chatdCtx, workspaceID.UUID) if err != nil { - p.logger.Warn(ctx, "failed to resolve workspace for recording", + warn("failed to resolve workspace for recording", slog.Error(err)) return result } @@ -99,7 +116,7 @@ func (p *Server) stopAndStoreRecording( var videoData, thumbnailData []byte for range maxParts { if ctx.Err() != nil { - p.logger.Warn(ctx, "context canceled while reading recording parts", slog.Error(ctx.Err())) + warn("context canceled while reading recording parts", slog.Error(ctx.Err())) break } @@ -108,7 +125,7 @@ func (p *Server) stopAndStoreRecording( break } if err != nil { - p.logger.Warn(ctx, "error reading next multipart part", slog.Error(err)) + warn("error reading next multipart part", slog.Error(err)) break } @@ -129,20 +146,20 @@ func (p *Server) stopAndStoreRecording( data, err := io.ReadAll(io.LimitReader(part, maxSize+1)) if err != nil { - p.logger.Warn(ctx, "failed to read recording part data", + warn("failed to read recording part data", slog.F("content_type", contentType), slog.Error(err)) continue } if int64(len(data)) > maxSize { - p.logger.Warn(ctx, "recording part exceeds maximum size, skipping", + warn("recording part exceeds maximum size, skipping", slog.F("content_type", contentType), slog.F("size", len(data)), slog.F("max_size", maxSize)) continue } if len(data) == 0 { - p.logger.Warn(ctx, "recording part is empty, skipping", + warn("recording part is empty, skipping", slog.F("content_type", contentType)) continue } @@ -150,13 +167,13 @@ func (p *Server) stopAndStoreRecording( switch contentType { case "video/mp4": if videoData != nil { - p.logger.Warn(ctx, "duplicate video/mp4 part in recording response, skipping") + warn("duplicate video/mp4 part in recording response, skipping") continue } videoData = data case "image/jpeg": if thumbnailData != nil { - p.logger.Warn(ctx, "duplicate image/jpeg part in recording response, skipping") + warn("duplicate image/jpeg part in recording response, skipping") continue } thumbnailData = data @@ -166,78 +183,76 @@ func (p *Server) stopAndStoreRecording( } } - // Second pass: store the collected data in the database and - // link it to the parent chat atomically. Insert + link must - // happen in the same transaction so that we never end up with - // chat_files rows that lack chat_file_links entries (which the - // purge job would treat as orphans and which users cannot view). - // Errors inside the transaction cause both inserts to be rolled - // back, so recording remains best-effort end-to-end. - txResult := struct { - recordingFileID string - thumbnailFileID string - }{} - //nolint:gocritic // AsChatd is required to insert and link chat files from the recording pipeline. - txErr := p.db.InTx(func(tx database.Store) error { - var fileIDs []uuid.UUID - if videoData != nil { - row, err := tx.InsertChatFile(dbauthz.AsChatd(ctx), database.InsertChatFileParams{ - OwnerID: ownerID, - OrganizationID: ws.OrganizationID, - Name: fmt.Sprintf("recording-%s.mp4", p.clock.Now().UTC().Format("2006-01-02T15-04-05Z")), - Mimetype: "video/mp4", - Data: videoData, - }) - if err != nil { - return xerrors.Errorf("insert recording: %w", err) - } - txResult.recordingFileID = row.ID.String() - fileIDs = append(fileIDs, row.ID) - } - if thumbnailData != nil && txResult.recordingFileID != "" { - row, err := tx.InsertChatFile(dbauthz.AsChatd(ctx), database.InsertChatFileParams{ - OwnerID: ownerID, - OrganizationID: ws.OrganizationID, - Name: fmt.Sprintf("thumbnail-%s.jpg", p.clock.Now().UTC().Format("2006-01-02T15-04-05Z")), - Mimetype: "image/jpeg", - Data: thumbnailData, - }) - if err != nil { - return xerrors.Errorf("insert thumbnail: %w", err) - } - txResult.thumbnailFileID = row.ID.String() - fileIDs = append(fileIDs, row.ID) - } - if len(fileIDs) == 0 { - return nil - } - - rejected, err := tx.LinkChatFiles(dbauthz.AsChatd(ctx), database.LinkChatFilesParams{ - ChatID: chatID, - MaxFileLinks: int32(codersdk.MaxChatFileIDs), - FileIds: fileIDs, - }) + // Second pass: store the collected data in the database. + if videoData != nil { + attachment, err := p.storeRecordingArtifact( + chatdCtx, + parentChatID, + ownerID, + ws.OrganizationID, + fmt.Sprintf("recording-%s.mp4", p.clock.Now().UTC().Format("2006-01-02T15-04-05Z")), + "video/mp4", + videoData, + ) if err != nil { - return xerrors.Errorf("link recording files: %w", err) + warn("failed to store recording in database", + slog.Error(err)) + } else { + result.recordingFileID = attachment.FileID.String() } - if rejected > 0 { - // The cap would be exceeded. Rolling back ensures the - // files are not persisted as orphans. MaxChatFileIDs is - // 20 today; hitting the cap with a 1-2 file batch means - // the chat is already saturated, and silently dropping - // the recording is preferable to leaving an unreachable - // blob. - return xerrors.Errorf("chat file link cap exceeded: %d file(s) rejected", rejected) - } - return nil - }, nil) - if txErr != nil { - p.logger.Warn(ctx, "failed to store and link recording", - slog.F("chat_id", chatID), - slog.Error(txErr)) - return result } - result.recordingFileID = txResult.recordingFileID - result.thumbnailFileID = txResult.thumbnailFileID + if thumbnailData != nil && result.recordingFileID != "" { + attachment, err := p.storeRecordingArtifact( + chatdCtx, + parentChatID, + ownerID, + ws.OrganizationID, + fmt.Sprintf("thumbnail-%s.jpg", p.clock.Now().UTC().Format("2006-01-02T15-04-05Z")), + "image/jpeg", + thumbnailData, + ) + if err != nil { + warn("failed to store thumbnail in database", + slog.Error(err)) + } else { + result.thumbnailFileID = attachment.FileID.String() + } + } + return result } + +func (p *Server) storeRecordingArtifact( + ctx context.Context, + chatID uuid.UUID, + ownerID uuid.UUID, + organizationID uuid.UUID, + name string, + mediaType string, + data []byte, +) (chattool.AttachmentMetadata, error) { + storedName, verifiedMediaType, err := chatfiles.PrepareRecordingArtifact(name, mediaType, data) + if err != nil { + return chattool.AttachmentMetadata{}, err + } + + var attachment chattool.AttachmentMetadata + err = p.db.InTx(func(tx database.Store) error { + var err error + attachment, err = storeLinkedChatFileTx( + ctx, + tx, + chatID, + ownerID, + organizationID, + storedName, + verifiedMediaType, + data, + ) + return err + }, database.DefaultTXOptions().WithID("store_recording_artifact")) + if err != nil { + return chattool.AttachmentMetadata{}, err + } + return attachment, nil +} diff --git a/coderd/x/chatd/recording_internal_test.go b/coderd/x/chatd/recording_internal_test.go index ccd52cbfad..e6b8f47c3d 100644 --- a/coderd/x/chatd/recording_internal_test.go +++ b/coderd/x/chatd/recording_internal_test.go @@ -62,6 +62,22 @@ func buildMultipartResponse(parts ...partSpec) workspacesdk.StopDesktopRecording } } +func validRecordingMP4(extra int, fill byte) []byte { + data := []byte{0x00, 0x00, 0x00, 0x18, 'f', 't', 'y', 'p', 'm', 'p', '4', '2', 0x00, 0x00, 0x00, 0x00, 'm', 'p', '4', '1', 'i', 's', 'o', 'm'} + if extra <= 0 { + return data + } + return append(data, bytes.Repeat([]byte{fill}, extra)...) +} + +func validRecordingJPEG(extra int, fill byte) []byte { + data := []byte{0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10, 'J', 'F', 'I', 'F', 0x00, 0x01, 0x01, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00} + if extra <= 0 { + return data + } + return append(data, bytes.Repeat([]byte{fill}, extra)...) +} + // createComputerUseParentChild creates a parent chat and a // computer_use child chat bound to the given workspace/agent. // Both chats are inserted directly via DB to avoid triggering @@ -191,7 +207,7 @@ func TestWaitAgentComputerUseRecording(t *testing.T) { setChatStatus(ctx, t, db, child.ID, database.ChatStatusWaiting, "") // Set up mock expectations for start and stop. - fakeMp4 := []byte("fake-mp4-data-for-recording-test") + fakeMp4 := validRecordingMP4(32, 0xA1) mockConn.EXPECT(). StartDesktopRecording(gomock.Any(), gomock.Any()). @@ -229,10 +245,14 @@ func TestWaitAgentComputerUseRecording(t *testing.T) { assert.Equal(t, user.ID, chatFile.OwnerID) assert.Equal(t, fakeMp4, chatFile.Data) - // Verify the file is linked to the parent chat. - linkedFiles, err := db.GetChatFileMetadataByChatID(ctx, parent.ID) + parentFiles, err := db.GetChatFileMetadataByChatID(ctx, parent.ID) require.NoError(t, err) - assert.Len(t, linkedFiles, 1) + require.Len(t, parentFiles, 1) + assert.Equal(t, fileUUID, parentFiles[0].ID) + + childFiles, err := db.GetChatFileMetadataByChatID(ctx, child.ID) + require.NoError(t, err) + assert.Empty(t, childFiles) } // TestWaitAgentComputerUseRecordingWithThumbnail verifies the @@ -268,8 +288,8 @@ func TestWaitAgentComputerUseRecordingWithThumbnail(t *testing.T) { setChatStatus(ctx, t, db, child.ID, database.ChatStatusWaiting, "") - fakeMp4 := []byte("fake-mp4-data-with-thumbnail-test") - fakeThumb := []byte("fake-jpeg-thumbnail-data") + fakeMp4 := validRecordingMP4(48, 0xA2) + fakeThumb := validRecordingJPEG(32, 0xB1) mockConn.EXPECT(). StartDesktopRecording(gomock.Any(), gomock.Any()). @@ -315,10 +335,15 @@ func TestWaitAgentComputerUseRecordingWithThumbnail(t *testing.T) { assert.Equal(t, "image/jpeg", thumbFile.Mimetype) assert.Equal(t, fakeThumb, thumbFile.Data) - // Verify both files are linked to the parent chat. - linkedFiles, err := db.GetChatFileMetadataByChatID(ctx, parent.ID) + parentFiles, err := db.GetChatFileMetadataByChatID(ctx, parent.ID) require.NoError(t, err) - assert.Len(t, linkedFiles, 2) + require.Len(t, parentFiles, 2) + assert.Equal(t, fileUUID, parentFiles[0].ID) + assert.Equal(t, thumbUUID, parentFiles[1].ID) + + childFiles, err := db.GetChatFileMetadataByChatID(ctx, child.ID) + require.NoError(t, err) + assert.Empty(t, childFiles) } // TestWaitAgentNonComputerUseNoRecording verifies that when the @@ -581,10 +606,11 @@ func TestStopAndStoreRecording_Oversized(t *testing.T) { ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - user, _, _ := seedInternalChatDeps(ctx, t, db) + user, org, model := seedInternalChatDeps(ctx, t, db) workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) // Build a streaming multipart response with a video/mp4 part // that exceeds MaxRecordingSize without allocating the full @@ -611,9 +637,8 @@ func TestStopAndStoreRecording_Oversized(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - uuid.Nil, ) assert.Empty(t, result.recordingFileID, "oversized recording should not be stored") } @@ -631,23 +656,12 @@ func TestStopAndStoreRecording_OversizedThumbnail(t *testing.T) { mockConn := agentconnmock.NewMockAgentConn(ctrl) user, org, model := seedInternalChatDeps(ctx, t, db) - workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) - chat, err := db.InsertChat(ctx, database.InsertChatParams{ - OrganizationID: org.ID, - OwnerID: user.ID, - WorkspaceID: uuid.NullUUID{UUID: workspace.ID, Valid: true}, - AgentID: uuid.NullUUID{UUID: agent.ID, Valid: true}, - LastModelConfigID: model.ID, - Title: "test-recording", - Status: database.ChatStatusPending, - ClientType: database.ChatClientTypeUi, - }) - require.NoError(t, err) - - videoData := bytes.Repeat([]byte{0xAA}, 1024) + videoData := validRecordingMP4(1024, 0xAA) // Build a streaming multipart response with a normal video part // and an oversized thumbnail part. @@ -677,9 +691,8 @@ func TestStopAndStoreRecording_OversizedThumbnail(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - chat.ID, ) // Video should be stored. @@ -692,11 +705,6 @@ func TestStopAndStoreRecording_OversizedThumbnail(t *testing.T) { // Thumbnail should be skipped (oversized). assert.Empty(t, result.thumbnailFileID, "oversized thumbnail should not be stored") - - // Verify that the stored files are linked to the chat. - linkedFiles, err := db.GetChatFileMetadataByChatID(ctx, chat.ID) - require.NoError(t, err) - assert.Len(t, linkedFiles, 1) } // TestStopAndStoreRecording_DuplicatePartsIgnored verifies that when @@ -712,24 +720,13 @@ func TestStopAndStoreRecording_DuplicatePartsIgnored(t *testing.T) { mockConn := agentconnmock.NewMockAgentConn(ctrl) user, org, model := seedInternalChatDeps(ctx, t, db) - workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) - chat, err := db.InsertChat(ctx, database.InsertChatParams{ - OrganizationID: org.ID, - OwnerID: user.ID, - WorkspaceID: uuid.NullUUID{UUID: workspace.ID, Valid: true}, - AgentID: uuid.NullUUID{UUID: agent.ID, Valid: true}, - LastModelConfigID: model.ID, - Title: "test-recording", - Status: database.ChatStatusPending, - ClientType: database.ChatClientTypeUi, - }) - require.NoError(t, err) - - firstVideo := bytes.Repeat([]byte{0x01}, 512) - secondVideo := bytes.Repeat([]byte{0x02}, 512) + firstVideo := validRecordingMP4(512, 0x01) + secondVideo := validRecordingMP4(512, 0x02) mockConn.EXPECT(). StopDesktopRecording(gomock.Any(), gomock.Any()). @@ -741,9 +738,8 @@ func TestStopAndStoreRecording_DuplicatePartsIgnored(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - chat.ID, ) // Only the first video part should be stored. @@ -752,11 +748,6 @@ func TestStopAndStoreRecording_DuplicatePartsIgnored(t *testing.T) { recFile, err := db.GetChatFileByID(ctx, recUUID) require.NoError(t, err) assert.Equal(t, firstVideo, recFile.Data, "first video part should be stored, not the duplicate") - - // Verify that the stored files are linked to the chat. - linkedFiles, err := db.GetChatFileMetadataByChatID(ctx, chat.ID) - require.NoError(t, err) - assert.Len(t, linkedFiles, 1) } // TestStopAndStoreRecording_Empty verifies that when the recording @@ -771,10 +762,11 @@ func TestStopAndStoreRecording_Empty(t *testing.T) { ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - user, _, _ := seedInternalChatDeps(ctx, t, db) + user, org, model := seedInternalChatDeps(ctx, t, db) workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) // Build a multipart response with an empty video/mp4 part. mockConn.EXPECT(). @@ -783,13 +775,66 @@ func TestStopAndStoreRecording_Empty(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - uuid.Nil, ) assert.Empty(t, result.recordingFileID, "empty recording should not be stored") } +// TestStopAndStoreRecording_LinkFailureRollsBackInsert verifies that a +// chat-file cap rejection does not leave behind an unlinked recording row. +func TestStopAndStoreRecording_LinkFailureRollsBackInsert(t *testing.T) { + t.Parallel() + + db, ps, sqlDB := dbtestutil.NewDBWithSQLDB(t) + ctx := chatdTestContext(t) + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + user, org, model := seedInternalChatDeps(ctx, t, db) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) + + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) + + for i := range codersdk.MaxChatFileIDs { + insertLinkedChatFile( + ctx, + t, + db, + parent.ID, + user.ID, + workspace.OrganizationID, + fmt.Sprintf("existing-%02d.txt", i), + "text/plain", + []byte("existing"), + ) + } + + var beforeCount int + require.NoError(t, sqlDB.QueryRowContext(ctx, "SELECT COUNT(*) FROM chat_files").Scan(&beforeCount)) + + videoData := validRecordingMP4(1000, 0xDE) + mockConn.EXPECT(). + StopDesktopRecording(gomock.Any(), gomock.Any()). + Return(buildMultipartResponse(partSpec{"video/mp4", videoData}), nil). + Times(1) + + recordingID := uuid.New().String() + result := server.stopAndStoreRecording( + ctx, mockConn, recordingID, parent.ID, user.ID, + uuid.NullUUID{UUID: workspace.ID, Valid: true}, + ) + + assert.Empty(t, result.recordingFileID) + assert.Empty(t, result.thumbnailFileID) + + var afterCount int + require.NoError(t, sqlDB.QueryRowContext(ctx, "SELECT COUNT(*) FROM chat_files").Scan(&afterCount)) + assert.Equal(t, beforeCount, afterCount) +} + // TestStopAndStoreRecording_WithThumbnail verifies that a multipart // response containing both a video/mp4 part and an image/jpeg part // results in both files being stored with correct mimetypes. @@ -803,24 +848,13 @@ func TestStopAndStoreRecording_WithThumbnail(t *testing.T) { mockConn := agentconnmock.NewMockAgentConn(ctrl) user, org, model := seedInternalChatDeps(ctx, t, db) - workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) - chat, err := db.InsertChat(ctx, database.InsertChatParams{ - OrganizationID: org.ID, - OwnerID: user.ID, - WorkspaceID: uuid.NullUUID{UUID: workspace.ID, Valid: true}, - AgentID: uuid.NullUUID{UUID: agent.ID, Valid: true}, - LastModelConfigID: model.ID, - Title: "test-recording", - Status: database.ChatStatusPending, - ClientType: database.ChatClientTypeUi, - }) - require.NoError(t, err) - - videoData := bytes.Repeat([]byte{0xDE, 0xAD}, 512) // 1024 bytes - thumbData := bytes.Repeat([]byte{0xFF, 0xD8}, 256) // 512 bytes + videoData := validRecordingMP4(1000, 0xDE) + thumbData := validRecordingJPEG(492, 0xD8) mockConn.EXPECT(). StopDesktopRecording(gomock.Any(), gomock.Any()). @@ -832,9 +866,8 @@ func TestStopAndStoreRecording_WithThumbnail(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - chat.ID, ) // Both file IDs should be valid UUIDs. @@ -854,104 +887,6 @@ func TestStopAndStoreRecording_WithThumbnail(t *testing.T) { require.NoError(t, err) assert.Equal(t, "image/jpeg", thumbFile.Mimetype) assert.Equal(t, thumbData, thumbFile.Data) - - // Verify that the stored files are linked to the chat. - linkedFiles, err := db.GetChatFileMetadataByChatID(ctx, chat.ID) - require.NoError(t, err) - assert.Len(t, linkedFiles, 2) -} - -// TestStopAndStoreRecording_CapExceededRollback verifies that when -// the chat's file-link cap would be exceeded by the new recording, -// stopAndStoreRecording rolls back the transaction so that the -// chat_files inserts are reverted. This prevents orphaned files -// that would be invisible to users and bypass the purge retention -// logic. -func TestStopAndStoreRecording_CapExceededRollback(t *testing.T) { - t.Parallel() - - db, ps := dbtestutil.NewDB(t) - ctx := chatdTestContext(t) - - ctrl := gomock.NewController(t) - mockConn := agentconnmock.NewMockAgentConn(ctrl) - - user, org, model := seedInternalChatDeps(ctx, t, db) - workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) - - server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) - - chat, err := db.InsertChat(ctx, database.InsertChatParams{ - OrganizationID: org.ID, - OwnerID: user.ID, - WorkspaceID: uuid.NullUUID{UUID: workspace.ID, Valid: true}, - AgentID: uuid.NullUUID{UUID: agent.ID, Valid: true}, - LastModelConfigID: model.ID, - Title: "test-recording", - Status: database.ChatStatusPending, - ClientType: database.ChatClientTypeUi, - }) - require.NoError(t, err) - - // Pre-fill the chat with MaxChatFileIDs links so any new - // recording file will be rejected by LinkChatFiles. - preFileIDs := make([]uuid.UUID, codersdk.MaxChatFileIDs) - for i := range preFileIDs { - row, insertErr := db.InsertChatFile(ctx, database.InsertChatFileParams{ - OwnerID: user.ID, - OrganizationID: org.ID, - Name: fmt.Sprintf("prefill-%d.bin", i), - Mimetype: "application/octet-stream", - Data: []byte{byte(i)}, - }) - require.NoError(t, insertErr) - preFileIDs[i] = row.ID - } - rejected, err := db.LinkChatFiles(ctx, database.LinkChatFilesParams{ - ChatID: chat.ID, - MaxFileLinks: int32(codersdk.MaxChatFileIDs), - FileIds: preFileIDs, - }) - require.NoError(t, err) - require.Zero(t, rejected) - - videoData := bytes.Repeat([]byte{0xAB}, 1024) - thumbData := bytes.Repeat([]byte{0xCD}, 512) - - mockConn.EXPECT(). - StopDesktopRecording(gomock.Any(), gomock.Any()). - Return(buildMultipartResponse( - partSpec{"video/mp4", videoData}, - partSpec{"image/jpeg", thumbData}, - ), nil). - Times(1) - - recordingID := uuid.New().String() - result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, - uuid.NullUUID{UUID: workspace.ID, Valid: true}, - chat.ID, - ) - - // Both IDs must be empty because the transaction was rolled - // back, so there is no file to reference. - assert.Empty(t, result.recordingFileID, - "recordingFileID must be empty when linking fails") - assert.Empty(t, result.thumbnailFileID, - "thumbnailFileID must be empty when linking fails") - - // Only the pre-fill files should exist; the recording and - // thumbnail rows must have been rolled back. - for _, id := range preFileIDs { - _, err := db.GetChatFileByID(ctx, id) - require.NoError(t, err, "prefill file must still exist") - } - - // Chat should still have exactly MaxChatFileIDs links; no - // new orphan or link was added. - linked, err := db.GetChatFileMetadataByChatID(ctx, chat.ID) - require.NoError(t, err) - assert.Len(t, linked, codersdk.MaxChatFileIDs) } // TestStopAndStoreRecording_VideoOnly verifies that a multipart @@ -967,23 +902,12 @@ func TestStopAndStoreRecording_VideoOnly(t *testing.T) { mockConn := agentconnmock.NewMockAgentConn(ctrl) user, org, model := seedInternalChatDeps(ctx, t, db) - workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) - chat, err := db.InsertChat(ctx, database.InsertChatParams{ - OrganizationID: org.ID, - OwnerID: user.ID, - WorkspaceID: uuid.NullUUID{UUID: workspace.ID, Valid: true}, - AgentID: uuid.NullUUID{UUID: agent.ID, Valid: true}, - LastModelConfigID: model.ID, - Title: "test-recording", - Status: database.ChatStatusPending, - ClientType: database.ChatClientTypeUi, - }) - require.NoError(t, err) - - videoData := make([]byte, 1024) + videoData := validRecordingMP4(1000, 0xCC) mockConn.EXPECT(). StopDesktopRecording(gomock.Any(), gomock.Any()). @@ -991,9 +915,8 @@ func TestStopAndStoreRecording_VideoOnly(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - chat.ID, ) // Recording should be stored. @@ -1007,11 +930,42 @@ func TestStopAndStoreRecording_VideoOnly(t *testing.T) { // No thumbnail. assert.Empty(t, result.thumbnailFileID, "ThumbnailFileID should be empty when no thumbnail part is present") +} - // Verify that the stored files are linked to the chat. - linkedFiles, err := db.GetChatFileMetadataByChatID(ctx, chat.ID) +// TestStopAndStoreRecording_MismatchedVideoBytesSkipped verifies that a +// part labeled video/mp4 is skipped when its bytes do not sniff as MP4. +func TestStopAndStoreRecording_MismatchedVideoBytesSkipped(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + ctx := chatdTestContext(t) + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + user, org, model := seedInternalChatDeps(ctx, t, db) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) + + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) + + mockConn.EXPECT(). + StopDesktopRecording(gomock.Any(), gomock.Any()). + Return(buildMultipartResponse(partSpec{"video/mp4", validRecordingJPEG(32, 0x44)}), nil). + Times(1) + + recordingID := uuid.New().String() + result := server.stopAndStoreRecording( + ctx, mockConn, recordingID, parent.ID, user.ID, + uuid.NullUUID{UUID: workspace.ID, Valid: true}, + ) + + assert.Empty(t, result.recordingFileID) + assert.Empty(t, result.thumbnailFileID) + + parentFiles, err := db.GetChatFileMetadataByChatID(ctx, parent.ID) require.NoError(t, err) - assert.Len(t, linkedFiles, 1) + assert.Empty(t, parentFiles) } // TestStopAndStoreRecording_DownloadFailure verifies that when @@ -1026,10 +980,11 @@ func TestStopAndStoreRecording_DownloadFailure(t *testing.T) { ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - user, _, _ := seedInternalChatDeps(ctx, t, db) + user, org, model := seedInternalChatDeps(ctx, t, db) workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) mockConn.EXPECT(). StopDesktopRecording(gomock.Any(), gomock.Any()). @@ -1038,9 +993,8 @@ func TestStopAndStoreRecording_DownloadFailure(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - uuid.Nil, ) assert.Empty(t, result.recordingFileID, "RecordingFileID should be empty on download failure") @@ -1060,24 +1014,13 @@ func TestStopAndStoreRecording_UnknownPartIgnored(t *testing.T) { mockConn := agentconnmock.NewMockAgentConn(ctrl) user, org, model := seedInternalChatDeps(ctx, t, db) - workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) - chat, err := db.InsertChat(ctx, database.InsertChatParams{ - OrganizationID: org.ID, - OwnerID: user.ID, - WorkspaceID: uuid.NullUUID{UUID: workspace.ID, Valid: true}, - AgentID: uuid.NullUUID{UUID: agent.ID, Valid: true}, - LastModelConfigID: model.ID, - Title: "test-recording", - Status: database.ChatStatusPending, - ClientType: database.ChatClientTypeUi, - }) - require.NoError(t, err) - - videoData := make([]byte, 1024) - thumbData := make([]byte, 512) + videoData := validRecordingMP4(1000, 0x11) + thumbData := validRecordingJPEG(492, 0x22) unknownData := make([]byte, 256) mockConn.EXPECT(). @@ -1090,9 +1033,8 @@ func TestStopAndStoreRecording_UnknownPartIgnored(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - chat.ID, ) // Both known parts should be stored. @@ -1112,11 +1054,6 @@ func TestStopAndStoreRecording_UnknownPartIgnored(t *testing.T) { require.NoError(t, err) assert.Equal(t, "image/jpeg", thumbFile.Mimetype) assert.Equal(t, thumbData, thumbFile.Data) - - // Verify that the stored files are linked to the chat. - linkedFiles, err := db.GetChatFileMetadataByChatID(ctx, chat.ID) - require.NoError(t, err) - assert.Len(t, linkedFiles, 2) } // TestStopAndStoreRecording_MalformedContentType verifies that a @@ -1130,10 +1067,11 @@ func TestStopAndStoreRecording_MalformedContentType(t *testing.T) { ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - user, _, _ := seedInternalChatDeps(ctx, t, db) + user, org, model := seedInternalChatDeps(ctx, t, db) workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) mockConn.EXPECT(). StopDesktopRecording(gomock.Any(), gomock.Any()). @@ -1145,9 +1083,8 @@ func TestStopAndStoreRecording_MalformedContentType(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - uuid.Nil, ) assert.Empty(t, result.recordingFileID, "RecordingFileID should be empty for malformed content type") @@ -1166,10 +1103,11 @@ func TestStopAndStoreRecording_MissingBoundary(t *testing.T) { ctrl := gomock.NewController(t) mockConn := agentconnmock.NewMockAgentConn(ctrl) - user, _, _ := seedInternalChatDeps(ctx, t, db) + user, org, model := seedInternalChatDeps(ctx, t, db) workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + parent, _ := createParentChildChats(ctx, t, server, user, org, model) mockConn.EXPECT(). StopDesktopRecording(gomock.Any(), gomock.Any()). @@ -1181,9 +1119,8 @@ func TestStopAndStoreRecording_MissingBoundary(t *testing.T) { recordingID := uuid.New().String() result := server.stopAndStoreRecording( - ctx, mockConn, recordingID, user.ID, + ctx, mockConn, recordingID, parent.ID, user.ID, uuid.NullUUID{UUID: workspace.ID, Valid: true}, - uuid.Nil, ) assert.Empty(t, result.recordingFileID, "RecordingFileID should be empty when boundary is missing") diff --git a/coderd/x/chatd/store_chat_attachment.go b/coderd/x/chatd/store_chat_attachment.go new file mode 100644 index 0000000000..7537a81f68 --- /dev/null +++ b/coderd/x/chatd/store_chat_attachment.go @@ -0,0 +1,111 @@ +package chatd + +import ( + "context" + + "github.com/google/uuid" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/chatfiles" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk" +) + +func (p *Server) newStoreChatAttachmentFunc(workspaceCtx *turnWorkspaceContext) chattool.StoreFileFunc { + return func( + ctx context.Context, + name string, + detectName string, + data []byte, + ) (chattool.AttachmentMetadata, error) { + workspaceCtx.chatStateMu.Lock() + chatSnapshot := *workspaceCtx.currentChat + workspaceCtx.chatStateMu.Unlock() + + return p.storeChatAttachment(ctx, chatSnapshot, name, detectName, data) + } +} + +func (p *Server) storeChatAttachment( + ctx context.Context, + chatSnapshot database.Chat, + name string, + detectName string, + data []byte, +) (chattool.AttachmentMetadata, error) { + if !chatSnapshot.WorkspaceID.Valid { + return chattool.AttachmentMetadata{}, xerrors.New("no workspace is associated with this chat. Use the create_workspace tool to create one") + } + + storedName, mediaType, err := chatfiles.PrepareStoredFile(name, detectName, data) + if err != nil { + return chattool.AttachmentMetadata{}, err + } + + // Insert and link in one transaction so a cap rejection or linking + // failure does not leave behind an unlinked chat file row. + var attachment chattool.AttachmentMetadata + err = p.db.InTx(func(tx database.Store) error { + ws, err := tx.GetWorkspaceByID(ctx, chatSnapshot.WorkspaceID.UUID) + if err != nil { + return xerrors.Errorf("resolve workspace: %w", err) + } + + attachment, err = storeLinkedChatFileTx( + ctx, + tx, + chatSnapshot.ID, + chatSnapshot.OwnerID, + ws.OrganizationID, + storedName, + mediaType, + data, + ) + return err + }, database.DefaultTXOptions().WithID("store_chat_attachment")) + if err != nil { + return chattool.AttachmentMetadata{}, err + } + return attachment, nil +} + +func storeLinkedChatFileTx( + ctx context.Context, + tx database.Store, + chatID uuid.UUID, + ownerID uuid.UUID, + organizationID uuid.UUID, + name string, + mediaType string, + data []byte, +) (chattool.AttachmentMetadata, error) { + row, err := tx.InsertChatFile(ctx, database.InsertChatFileParams{ + OwnerID: ownerID, + OrganizationID: organizationID, + Name: name, + Mimetype: mediaType, + Data: data, + }) + if err != nil { + return chattool.AttachmentMetadata{}, xerrors.Errorf("insert chat file: %w", err) + } + + rejected, err := tx.LinkChatFiles(ctx, database.LinkChatFilesParams{ + ChatID: chatID, + MaxFileLinks: int32(codersdk.MaxChatFileIDs), + FileIds: []uuid.UUID{row.ID}, + }) + if err != nil { + return chattool.AttachmentMetadata{}, xerrors.Errorf("link chat file: %w", err) + } + if rejected > 0 { + return chattool.AttachmentMetadata{}, xerrors.Errorf("chat already has the maximum of %d linked files", codersdk.MaxChatFileIDs) + } + + return chattool.AttachmentMetadata{ + FileID: row.ID, + MediaType: mediaType, + Name: name, + }, nil +} diff --git a/coderd/x/chatd/store_chat_attachment_test.go b/coderd/x/chatd/store_chat_attachment_test.go new file mode 100644 index 0000000000..e8fef3a627 --- /dev/null +++ b/coderd/x/chatd/store_chat_attachment_test.go @@ -0,0 +1,267 @@ +package chatd //nolint:testpackage + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/coder/coder/v2/coderd/chatfiles" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbmock" + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk" +) + +func TestStoreChatAttachment_Success(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + tx := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + chatID := uuid.New() + ownerID := uuid.New() + workspaceID := uuid.New() + orgID := uuid.New() + fileID := uuid.New() + chatSnapshot := database.Chat{ + ID: chatID, + OwnerID: ownerID, + WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, + } + + expectStoreChatAttachmentTx(t, db, tx) + tx.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ID: workspaceID, OrganizationID: orgID}, nil) + tx.EXPECT().InsertChatFile(gomock.Any(), gomock.AssignableToTypeOf(database.InsertChatFileParams{})).DoAndReturn( + func(_ context.Context, arg database.InsertChatFileParams) (database.InsertChatFileRow, error) { + require.Equal(t, ownerID, arg.OwnerID) + require.Equal(t, orgID, arg.OrganizationID) + require.Equal(t, "build.log", arg.Name) + require.Equal(t, "text/plain", arg.Mimetype) + require.Equal(t, []byte("build output"), arg.Data) + return database.InsertChatFileRow{ID: fileID}, nil + }, + ) + tx.EXPECT().LinkChatFiles(gomock.Any(), database.LinkChatFilesParams{ + ChatID: chatID, + MaxFileLinks: int32(codersdk.MaxChatFileIDs), + FileIds: []uuid.UUID{fileID}, + }).Return(int32(0), nil) + + attachment, err := server.storeChatAttachment(context.Background(), chatSnapshot, "build.log", "build.log", []byte("build output")) + require.NoError(t, err) + require.Equal(t, chattool.AttachmentMetadata{ + FileID: fileID, + MediaType: "text/plain", + Name: "build.log", + }, attachment) +} + +func TestStoreChatAttachment_UsesDetectNameForClassification(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + tx := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + chatID := uuid.New() + ownerID := uuid.New() + workspaceID := uuid.New() + orgID := uuid.New() + fileID := uuid.New() + chatSnapshot := database.Chat{ + ID: chatID, + OwnerID: ownerID, + WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, + } + + expectStoreChatAttachmentTx(t, db, tx) + tx.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ID: workspaceID, OrganizationID: orgID}, nil) + tx.EXPECT().InsertChatFile(gomock.Any(), gomock.AssignableToTypeOf(database.InsertChatFileParams{})).DoAndReturn( + func(_ context.Context, arg database.InsertChatFileParams) (database.InsertChatFileRow, error) { + require.Equal(t, "payload.txt", arg.Name) + require.Equal(t, "application/json", arg.Mimetype) + return database.InsertChatFileRow{ID: fileID}, nil + }, + ) + tx.EXPECT().LinkChatFiles(gomock.Any(), database.LinkChatFilesParams{ + ChatID: chatID, + MaxFileLinks: int32(codersdk.MaxChatFileIDs), + FileIds: []uuid.UUID{fileID}, + }).Return(int32(0), nil) + + attachment, err := server.storeChatAttachment(context.Background(), chatSnapshot, "payload.txt", "report.json", []byte(`{"ok":true}`)) + require.NoError(t, err) + require.Equal(t, "payload.txt", attachment.Name) + require.Equal(t, "application/json", attachment.MediaType) +} + +func TestStoreChatAttachment_RejectsUnsupportedStoredFileTypeBeforeDBWork(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + chatSnapshot := database.Chat{ + ID: uuid.New(), + OwnerID: uuid.New(), + WorkspaceID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, + } + + attachment, err := server.storeChatAttachment( + context.Background(), + chatSnapshot, + "evil.svg", + "evil.svg", + []byte(``), + ) + require.ErrorIs(t, err, chatfiles.ErrUnsupportedStoredFileType) + require.ErrorContains(t, err, "image/svg+xml") + require.Equal(t, chattool.AttachmentMetadata{}, attachment) +} + +func TestStoreChatAttachment_NoWorkspace(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + attachment, err := server.storeChatAttachment(context.Background(), database.Chat{}, "build.log", "build.log", []byte("build output")) + require.ErrorContains(t, err, "no workspace is associated") + require.Equal(t, chattool.AttachmentMetadata{}, attachment) +} + +func TestStoreChatAttachment_WorkspaceLookupError(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + tx := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + workspaceID := uuid.New() + chatSnapshot := database.Chat{ + ID: uuid.New(), + OwnerID: uuid.New(), + WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, + } + + expectStoreChatAttachmentTx(t, db, tx) + tx.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{}, context.DeadlineExceeded) + + attachment, err := server.storeChatAttachment(context.Background(), chatSnapshot, "build.log", "build.log", []byte("build output")) + require.ErrorContains(t, err, "resolve workspace") + require.ErrorIs(t, err, context.DeadlineExceeded) + require.Equal(t, chattool.AttachmentMetadata{}, attachment) +} + +func TestStoreChatAttachment_InsertError(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + tx := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + workspaceID := uuid.New() + chatSnapshot := database.Chat{ + ID: uuid.New(), + OwnerID: uuid.New(), + WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, + } + + expectStoreChatAttachmentTx(t, db, tx) + tx.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ID: workspaceID, OrganizationID: uuid.New()}, nil) + tx.EXPECT().InsertChatFile(gomock.Any(), gomock.Any()).Return(database.InsertChatFileRow{}, context.DeadlineExceeded) + + attachment, err := server.storeChatAttachment(context.Background(), chatSnapshot, "build.log", "build.log", []byte("build output")) + require.ErrorContains(t, err, "insert chat file") + require.ErrorIs(t, err, context.DeadlineExceeded) + require.Equal(t, chattool.AttachmentMetadata{}, attachment) +} + +func TestStoreChatAttachment_StrictCapError(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + tx := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + chatID := uuid.New() + ownerID := uuid.New() + workspaceID := uuid.New() + orgID := uuid.New() + fileID := uuid.New() + chatSnapshot := database.Chat{ + ID: chatID, + OwnerID: ownerID, + WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, + } + + expectStoreChatAttachmentTx(t, db, tx) + tx.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ID: workspaceID, OrganizationID: orgID}, nil) + tx.EXPECT().InsertChatFile(gomock.Any(), gomock.AssignableToTypeOf(database.InsertChatFileParams{})).Return(database.InsertChatFileRow{ID: fileID}, nil) + tx.EXPECT().LinkChatFiles(gomock.Any(), database.LinkChatFilesParams{ + ChatID: chatID, + MaxFileLinks: int32(codersdk.MaxChatFileIDs), + FileIds: []uuid.UUID{fileID}, + }).Return(int32(1), nil) + + attachment, err := server.storeChatAttachment(context.Background(), chatSnapshot, "build.log", "build.log", []byte("build output")) + require.ErrorContains(t, err, "chat already has the maximum of 20 linked files") + require.Equal(t, chattool.AttachmentMetadata{}, attachment) +} + +func TestStoreChatAttachment_LinkError(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + tx := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + + chatID := uuid.New() + ownerID := uuid.New() + workspaceID := uuid.New() + orgID := uuid.New() + fileID := uuid.New() + chatSnapshot := database.Chat{ + ID: chatID, + OwnerID: ownerID, + WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}, + } + + expectStoreChatAttachmentTx(t, db, tx) + tx.EXPECT().GetWorkspaceByID(gomock.Any(), workspaceID).Return(database.Workspace{ID: workspaceID, OrganizationID: orgID}, nil) + tx.EXPECT().InsertChatFile(gomock.Any(), gomock.Any()).Return(database.InsertChatFileRow{ID: fileID}, nil) + tx.EXPECT().LinkChatFiles(gomock.Any(), database.LinkChatFilesParams{ + ChatID: chatID, + MaxFileLinks: int32(codersdk.MaxChatFileIDs), + FileIds: []uuid.UUID{fileID}, + }).Return(int32(0), context.DeadlineExceeded) + + attachment, err := server.storeChatAttachment(context.Background(), chatSnapshot, "build.log", "build.log", []byte("build output")) + require.ErrorContains(t, err, "link chat file") + require.ErrorIs(t, err, context.DeadlineExceeded) + require.Equal(t, chattool.AttachmentMetadata{}, attachment) +} + +func expectStoreChatAttachmentTx(t *testing.T, db, tx *dbmock.MockStore) { + t.Helper() + + db.EXPECT().InTx(gomock.Any(), gomock.AssignableToTypeOf(&database.TxOptions{})).DoAndReturn( + func(fn func(database.Store) error, opts *database.TxOptions) error { + require.NotNil(t, opts) + require.Equal(t, "store_chat_attachment", opts.TxIdentifier) + return fn(tx) + }, + ) +} diff --git a/coderd/x/chatd/subagent.go b/coderd/x/chatd/subagent.go index a6898f9220..d632bcdbad 100644 --- a/coderd/x/chatd/subagent.go +++ b/coderd/x/chatd/subagent.go @@ -41,6 +41,8 @@ Your primary tool is the "computer" tool which lets you interact with the deskto Guidelines: - Always start by taking a screenshot to see the current state of the desktop. +- Use wait or ordinary actions when you only need a screenshot for your own reasoning. +- Use an explicit screenshot action when you want to share a durable screenshot with the user; those screenshots are attached to the chat automatically. - Be precise with coordinates when clicking or typing. - Wait for UI elements to load before interacting with them. - If an action doesn't produce the expected result, try alternative approaches. @@ -373,7 +375,7 @@ func (p *Server) subagentTools( stopCtx, stopCancel := context.WithTimeout(context.WithoutCancel(ctx), 90*time.Second) defer stopCancel() recResult = p.stopAndStoreRecording(stopCtx, agentConn, - recordingID, parent.OwnerID, parent.WorkspaceID, parent.ID) + recordingID, parent.ID, parent.OwnerID, parent.WorkspaceID) } resp := map[string]any{ "chat_id": targetChatID.String(), diff --git a/coderd/x/chatd/subagent_internal_test.go b/coderd/x/chatd/subagent_internal_test.go index 3d5dac0a03..f1ffb277f4 100644 --- a/coderd/x/chatd/subagent_internal_test.go +++ b/coderd/x/chatd/subagent_internal_test.go @@ -19,6 +19,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/pubsub" coderdpubsub "github.com/coder/coder/v2/coderd/pubsub" + "github.com/coder/coder/v2/coderd/x/chatd/chatloop" "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" "github.com/coder/coder/v2/coderd/x/chatd/chattool" @@ -27,16 +28,6 @@ import ( "github.com/coder/quartz" ) -func TestComputerUseSubagentSystemPrompt(t *testing.T) { - t.Parallel() - - // Verify the system prompt constant is non-empty and contains - // key instructions for the computer use agent. - assert.NotEmpty(t, computerUseSubagentSystemPrompt) - assert.Contains(t, computerUseSubagentSystemPrompt, "computer") - assert.Contains(t, computerUseSubagentSystemPrompt, "screenshot") -} - func TestSubagentFallbackChatTitle(t *testing.T) { t.Parallel() @@ -1291,6 +1282,158 @@ func insertAssistantMessage( require.NoError(t, err) } +func insertLinkedChatFile( + ctx context.Context, + t *testing.T, + db database.Store, + chatID uuid.UUID, + ownerID uuid.UUID, + organizationID uuid.UUID, + name string, + mediaType string, + data []byte, +) uuid.UUID { + t.Helper() + + file, err := db.InsertChatFile(ctx, database.InsertChatFileParams{ + OwnerID: ownerID, + OrganizationID: organizationID, + Name: name, + Mimetype: mediaType, + Data: data, + }) + require.NoError(t, err) + + rejected, err := db.LinkChatFiles(ctx, database.LinkChatFilesParams{ + ChatID: chatID, + MaxFileLinks: int32(codersdk.MaxChatFileIDs), + FileIds: []uuid.UUID{file.ID}, + }) + require.NoError(t, err) + require.Zero(t, rejected) + + return file.ID +} + +func TestWaitAgentDoesNotRelayComputerUseSubagentAttachments(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + ctx := chatdTestContext(t) + user, org, model := seedInternalChatDeps(ctx, t, db) + workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + + parent, child := createComputerUseParentChild( + ctx, t, server, user, org, model, workspace, agent, + "parent-relay", "child-relay", + ) + + insertedFile := insertLinkedChatFile( + ctx, + t, + db, + child.ID, + user.ID, + workspace.OrganizationID, + "screenshot.png", + "image/png", + []byte("fake-png"), + ) + insertAssistantMessage(ctx, t, db, child.ID, model.ID, "Shared the screenshot.") + setChatStatus(ctx, t, db, child.ID, database.ChatStatusWaiting, "") + + resp, err := invokeWaitAgentTool(ctx, t, server, db, parent.ID, child.ID, 5) + require.NoError(t, err) + require.False(t, resp.IsError, "expected successful response, got: %s", resp.Content) + + var result map[string]any + require.NoError(t, json.Unmarshal([]byte(resp.Content), &result)) + require.Equal(t, "Shared the screenshot.", result["report"]) + require.Equal(t, string(database.ChatStatusWaiting), result["status"]) + assert.NotContains(t, result, "attachment_count") + assert.NotContains(t, result, "attachment_warning") + + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + assert.Empty(t, attachments) + parts := buildAssistantPartsForPersist( + context.Background(), + testutil.Logger(t), + nil, + []fantasy.ToolResultContent{{ + ToolCallID: "call-1", + ToolName: "wait_agent", + ClientMetadata: resp.Metadata, + }}, + chatloop.PersistedStep{}, + nil, + ) + assert.Empty(t, parts) + + parentFiles, err := db.GetChatFileMetadataByChatID(ctx, parent.ID) + require.NoError(t, err) + assert.Empty(t, parentFiles) + + childFiles, err := db.GetChatFileMetadataByChatID(ctx, child.ID) + require.NoError(t, err) + require.Len(t, childFiles, 1) + assert.Equal(t, insertedFile, childFiles[0].ID) + assert.Equal(t, "screenshot.png", childFiles[0].Name) + assert.Equal(t, "image/png", childFiles[0].Mimetype) +} + +func TestWaitAgentDoesNotRelayRegularSubagentAttachments(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + ctx := chatdTestContext(t) + user, org, model := seedInternalChatDeps(ctx, t, db) + workspace, _, _ := seedWorkspaceBinding(t, db, user.ID) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + + parent, child := createParentChildChats(ctx, t, server, user, org, model) + server.drainInflight() + + insertedFile := insertLinkedChatFile( + ctx, + t, + db, + child.ID, + user.ID, + workspace.OrganizationID, + "notes.txt", + "text/plain", + []byte("release notes"), + ) + insertAssistantMessage(ctx, t, db, child.ID, model.ID, "Shared the release notes.") + setChatStatus(ctx, t, db, child.ID, database.ChatStatusWaiting, "") + + resp, err := invokeWaitAgentTool(ctx, t, server, db, parent.ID, child.ID, 5) + require.NoError(t, err) + require.False(t, resp.IsError, "expected successful response, got: %s", resp.Content) + + var result map[string]any + require.NoError(t, json.Unmarshal([]byte(resp.Content), &result)) + require.Equal(t, "Shared the release notes.", result["report"]) + assert.NotContains(t, result, "attachment_count") + assert.NotContains(t, result, "attachment_warning") + attachments, err := chattool.AttachmentsFromMetadata(resp.Metadata) + require.NoError(t, err) + assert.Empty(t, attachments) + + parentFiles, err := db.GetChatFileMetadataByChatID(ctx, parent.ID) + require.NoError(t, err) + assert.Empty(t, parentFiles) + + childFiles, err := db.GetChatFileMetadataByChatID(ctx, child.ID) + require.NoError(t, err) + require.Len(t, childFiles, 1) + assert.Equal(t, insertedFile, childFiles[0].ID) + assert.Equal(t, "notes.txt", childFiles[0].Name) + assert.Equal(t, "text/plain", childFiles[0].Mimetype) +} + func TestAwaitSubagentCompletion(t *testing.T) { t.Parallel() diff --git a/codersdk/chats.go b/codersdk/chats.go index d798e27d16..3285df8c7b 100644 --- a/codersdk/chats.go +++ b/codersdk/chats.go @@ -212,6 +212,7 @@ type ChatMessagePart struct { URL string `json:"url" variants:"source"` Title string `json:"title,omitempty" variants:"source?"` MediaType string `json:"media_type" variants:"file"` + Name string `json:"name,omitempty" variants:"file?"` Data []byte `json:"data,omitempty" variants:"file?"` FileID uuid.NullUUID `json:"file_id,omitempty" format:"uuid" variants:"file?"` FileName string `json:"file_name" variants:"file-reference"` @@ -327,11 +328,12 @@ func ChatMessageToolResult(toolCallID, toolName string, result json.RawMessage, } // ChatMessageFile builds a file chat message part. -func ChatMessageFile(fileID uuid.UUID, mediaType string) ChatMessagePart { +func ChatMessageFile(fileID uuid.UUID, mediaType string, name string) ChatMessagePart { return ChatMessagePart{ Type: ChatMessagePartTypeFile, FileID: uuid.NullUUID{UUID: fileID, Valid: true}, MediaType: mediaType, + Name: name, } } diff --git a/go.mod b/go.mod index d8090c0a88..69cdd26e09 100644 --- a/go.mod +++ b/go.mod @@ -324,7 +324,7 @@ require ( github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect github.com/felixge/httpsnoop v1.0.4 // indirect github.com/fxamacker/cbor/v2 v2.9.0 // indirect - github.com/gabriel-vasile/mimetype v1.4.12 // indirect + github.com/gabriel-vasile/mimetype v1.4.12 github.com/go-chi/hostrouter v0.3.0 // indirect github.com/go-ini/ini v1.67.0 // indirect github.com/go-logr/stdr v1.2.2 // indirect diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index b9338aea5a..fe7912e662 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1614,6 +1614,7 @@ export interface ChatFileMetadata { export interface ChatFilePart { readonly type: "file"; readonly media_type: string; + readonly name?: string; readonly data?: string; readonly file_id?: string; }