diff --git a/pkg/app/app.go b/pkg/app/app.go index 4e1972757..3d5d1cfbc 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -6,7 +6,6 @@ import ( "log/slog" "os" "os/exec" - "path/filepath" "slices" "strings" "sync/atomic" @@ -239,7 +238,7 @@ func (a *App) EmitStartupInfo(ctx context.Context, events chan runtime.Event) { } // Run one agent loop -func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string, attachments map[string]string) { +func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string, attachments []messages.Attachment) { a.cancel = cancel // If this is the first message and no title exists, start local title generation @@ -256,87 +255,19 @@ func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string var textBuilder strings.Builder textBuilder.WriteString(message) - // multiContent holds the combined text part plus any binary file parts. + // binaryParts holds non-text file parts (images, PDFs, etc.) var binaryParts []chat.MessagePart - // Attachments are keyed by @filepath placeholder. - for placeholder := range attachments { - filePath := strings.TrimPrefix(placeholder, "@") - if filePath == "" { - slog.Debug("skipping attachment with empty file path", "placeholder", placeholder) - continue - } - - absPath, err := filepath.Abs(filePath) - if err != nil { - slog.Warn("skipping attachment: invalid path", "path", filePath, "error", err) - a.events <- runtime.Warning(fmt.Sprintf("Skipped attachment %s: invalid path", filePath), "") - continue - } - - fi, err := os.Stat(absPath) - if err != nil { - var reason string - switch { - case os.IsNotExist(err): - reason = "file does not exist" - case os.IsPermission(err): - reason = "permission denied" - default: - reason = fmt.Sprintf("cannot access file: %v", err) - } - slog.Warn("skipping attachment", "path", absPath, "reason", reason) - a.events <- runtime.Warning(fmt.Sprintf("Skipped attachment %s: %s", filePath, reason), "") - continue - } - - if !fi.Mode().IsRegular() { - slog.Warn("skipping attachment: not a regular file", "path", absPath, "mode", fi.Mode().String()) - a.events <- runtime.Warning(fmt.Sprintf("Skipped attachment %s: not a regular file", filePath), "") - continue - } - - const maxAttachmentSize = 100 * 1024 * 1024 // 100MB - if fi.Size() > maxAttachmentSize { - slog.Warn("skipping attachment: file too large", "path", absPath, "size", fi.Size(), "max", maxAttachmentSize) - a.events <- runtime.Warning(fmt.Sprintf("Skipped attachment %s: file too large (max 100MB)", filePath), "") - continue - } - - mimeType := chat.DetectMimeType(absPath) - + for _, att := range attachments { switch { - case chat.IsTextFile(absPath): - // Text files are appended to the message text so the model - // sees them in a single content block alongside the user's question. - if fi.Size() > chat.MaxInlineFileSize { - slog.Warn("skipping attachment: text file too large to inline", "path", absPath, "size", fi.Size(), "max", chat.MaxInlineFileSize) - a.events <- runtime.Warning(fmt.Sprintf("Skipped attachment %s: text file too large to inline (max 1MB)", filePath), "") - continue - } - content, err := chat.ReadFileForInline(absPath) - if err != nil { - slog.Warn("skipping attachment: failed to read file", "path", absPath, "error", err) - a.events <- runtime.Warning(fmt.Sprintf("Skipped attachment %s: failed to read file", filePath), "") - continue - } - textBuilder.WriteString("\n\n") - textBuilder.WriteString(content) - - case chat.IsSupportedMimeType(mimeType): - // Binary files (images, PDFs) are kept as separate file parts. - binaryParts = append(binaryParts, chat.MessagePart{ - Type: chat.MessagePartTypeFile, - File: &chat.MessageFile{ - Path: absPath, - MimeType: mimeType, - }, - }) - + case att.FilePath != "": + // File-reference attachment: read and classify from disk. + a.processFileAttachment(ctx, att, &textBuilder, &binaryParts) + case att.Content != "": + // Inline content attachment (e.g. pasted text). + a.processInlineAttachment(att, &textBuilder) default: - slog.Warn("skipping attachment: unsupported file type", "path", absPath, "mime_type", mimeType) - a.events <- runtime.Warning(fmt.Sprintf("Skipped attachment %s: unsupported file type", filePath), "") - continue + slog.Debug("skipping attachment with no file path or content", "name", att.Name) } } @@ -361,11 +292,94 @@ func (a *App) Run(ctx context.Context, cancel context.CancelFunc, message string a.titleGenerating.Store(false) } - a.events <- event + a.sendEvent(ctx, event) } }() } +// processFileAttachment reads a file from disk, classifies it, and either +// appends its text content to textBuilder or adds a binary part to binaryParts. +func (a *App) processFileAttachment(ctx context.Context, att messages.Attachment, textBuilder *strings.Builder, binaryParts *[]chat.MessagePart) { + absPath := att.FilePath + + fi, err := os.Stat(absPath) + if err != nil { + var reason string + switch { + case os.IsNotExist(err): + reason = "file does not exist" + case os.IsPermission(err): + reason = "permission denied" + default: + reason = fmt.Sprintf("cannot access file: %v", err) + } + slog.Warn("skipping attachment", "path", absPath, "reason", reason) + a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: %s", att.Name, reason), "")) + return + } + + if !fi.Mode().IsRegular() { + slog.Warn("skipping attachment: not a regular file", "path", absPath, "mode", fi.Mode().String()) + a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: not a regular file", att.Name), "")) + return + } + + const maxAttachmentSize = 100 * 1024 * 1024 // 100MB + if fi.Size() > maxAttachmentSize { + slog.Warn("skipping attachment: file too large", "path", absPath, "size", fi.Size(), "max", maxAttachmentSize) + a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: file too large (max 100MB)", att.Name), "")) + return + } + + mimeType := chat.DetectMimeType(absPath) + + switch { + case chat.IsTextFile(absPath): + if fi.Size() > chat.MaxInlineFileSize { + slog.Warn("skipping attachment: text file too large to inline", "path", absPath, "size", fi.Size(), "max", chat.MaxInlineFileSize) + a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: text file too large to inline (max 5MB)", att.Name), "")) + return + } + content, err := chat.ReadFileForInline(absPath) + if err != nil { + slog.Warn("skipping attachment: failed to read file", "path", absPath, "error", err) + a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: failed to read file", att.Name), "")) + return + } + textBuilder.WriteString("\n\n") + textBuilder.WriteString(content) + + case chat.IsSupportedMimeType(mimeType): + *binaryParts = append(*binaryParts, chat.MessagePart{ + Type: chat.MessagePartTypeFile, + File: &chat.MessageFile{ + Path: absPath, + MimeType: mimeType, + }, + }) + + default: + slog.Warn("skipping attachment: unsupported file type", "path", absPath, "mime_type", mimeType) + a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: unsupported file type", att.Name), "")) + } +} + +// sendEvent sends an event to the TUI, respecting context cancellation to +// avoid blocking on the channel when the consumer has stopped reading. +func (a *App) sendEvent(ctx context.Context, event tea.Msg) { + select { + case a.events <- event: + case <-ctx.Done(): + } +} + +// processInlineAttachment handles content that is already in memory (e.g. pasted +// text). The content is appended to textBuilder wrapped in an XML tag for context. +func (a *App) processInlineAttachment(att messages.Attachment, textBuilder *strings.Builder) { + textBuilder.WriteString("\n\n") + fmt.Fprintf(textBuilder, "\n%s\n", att.Name, att.Content) +} + // RunWithMessage runs the agent loop with a pre-constructed message. // This is used for special cases like image attachments. func (a *App) RunWithMessage(ctx context.Context, cancel context.CancelFunc, msg *session.Message) { @@ -401,7 +415,7 @@ func (a *App) RunWithMessage(ctx context.Context, cancel context.CancelFunc, msg a.titleGenerating.Store(false) } - a.events <- event + a.sendEvent(ctx, event) } }() } diff --git a/pkg/tui/components/editor/editor.go b/pkg/tui/components/editor/editor.go index f5283e8f0..78d11dc5d 100644 --- a/pkg/tui/components/editor/editor.go +++ b/pkg/tui/components/editor/editor.go @@ -1319,11 +1319,21 @@ func (e *editor) tryAddFileRef(word string) { } // addFileAttachment adds a file reference as an attachment if valid. +// The path is resolved to an absolute path so downstream consumers +// (e.g. processFileAttachment) always receive a fully qualified path. func (e *editor) addFileAttachment(placeholder string) { path := strings.TrimPrefix(placeholder, "@") + // Resolve to absolute path so the attachment carries a fully qualified + // path regardless of the working directory at send time. + absPath, err := filepath.Abs(path) + if err != nil { + slog.Warn("skipping attachment: cannot resolve path", "path", path, "error", err) + return + } + // Check if it's an existing file (not directory) - info, err := os.Stat(path) + info, err := os.Stat(absPath) if err != nil || info.IsDir() { return } @@ -1336,22 +1346,25 @@ func (e *editor) addFileAttachment(placeholder string) { } e.attachments = append(e.attachments, attachment{ - path: path, + path: absPath, placeholder: placeholder, - label: fmt.Sprintf("%s (%s)", filepath.Base(path), units.HumanSize(float64(info.Size()))), + label: fmt.Sprintf("%s (%s)", filepath.Base(absPath), units.HumanSize(float64(info.Size()))), sizeBytes: int(info.Size()), isTemp: false, }) } -// collectAttachments returns a map of placeholder to file content for all attachments -// referenced in content. Unreferenced attachments are cleaned up. -func (e *editor) collectAttachments(content string) map[string]string { +// collectAttachments returns structured attachments for all items referenced in +// content. For paste attachments the content is read into memory (the backing +// temp file is removed). For file-reference attachments the path is preserved +// so the consumer can read and classify the file (e.g. detect MIME type). +// Unreferenced attachments are cleaned up. +func (e *editor) collectAttachments(content string) []messages.Attachment { if len(e.attachments) == 0 { return nil } - attachments := make(map[string]string) + var result []messages.Attachment for _, att := range e.attachments { if !strings.Contains(content, att.placeholder) { if att.isTemp { @@ -1360,24 +1373,29 @@ func (e *editor) collectAttachments(content string) map[string]string { continue } - data, err := os.ReadFile(att.path) - if err != nil { - slog.Warn("failed to read attachment", "path", att.path, "error", err) - if att.isTemp { - _ = os.Remove(att.path) - } - continue - } - - attachments[att.placeholder] = string(data) - if att.isTemp { + // Paste attachment: read into memory and remove the temp file. + data, err := os.ReadFile(att.path) _ = os.Remove(att.path) + if err != nil { + slog.Warn("failed to read paste attachment", "path", att.path, "error", err) + continue + } + result = append(result, messages.Attachment{ + Name: strings.TrimPrefix(att.placeholder, "@"), + Content: string(data), + }) + } else { + // File-reference attachment: keep the path for later processing. + result = append(result, messages.Attachment{ + Name: filepath.Base(att.path), + FilePath: att.path, + }) } } e.attachments = nil - return attachments + return result } // Cleanup removes any temporary paste files that haven't been sent yet. diff --git a/pkg/tui/components/editor/editor_test.go b/pkg/tui/components/editor/editor_test.go index 24171d257..d02d79a2b 100644 --- a/pkg/tui/components/editor/editor_test.go +++ b/pkg/tui/components/editor/editor_test.go @@ -32,7 +32,7 @@ func TestCollectAttachments(t *testing.T) { assert.Nil(t, result) }) - t.Run("file content in attachments map", func(t *testing.T) { + t.Run("file reference preserves path", func(t *testing.T) { t.Parallel() // Create a temp file @@ -51,8 +51,10 @@ func TestCollectAttachments(t *testing.T) { result := e.collectAttachments(content) - require.NotNil(t, result) - assert.Equal(t, "file content here", result[ref]) + require.Len(t, result, 1) + assert.Equal(t, "test.txt", result[0].Name) + assert.Equal(t, tmpFile, result[0].FilePath) + assert.Empty(t, result[0].Content, "file refs should not have inline content") assert.Nil(t, e.attachments, "attachments should be cleared after collection") }) @@ -75,9 +77,9 @@ func TestCollectAttachments(t *testing.T) { result := e.collectAttachments(content) - require.NotNil(t, result) - assert.Equal(t, "package first", result[ref1]) - assert.Equal(t, "package second", result[ref2]) + require.Len(t, result, 2) + assert.Equal(t, file1, result[0].FilePath) + assert.Equal(t, file2, result[1].FilePath) }) t.Run("skips refs not in content", func(t *testing.T) { @@ -97,11 +99,11 @@ func TestCollectAttachments(t *testing.T) { result := e.collectAttachments(content) - assert.Empty(t, result, "should return empty map when ref not in content") + assert.Empty(t, result, "should return empty when ref not in content") assert.Nil(t, e.attachments, "attachments should be cleared after collection") }) - t.Run("skips nonexistent files", func(t *testing.T) { + t.Run("file reference passes through even for nonexistent files", func(t *testing.T) { t.Parallel() ref := "@/nonexistent/path/file.txt" @@ -114,8 +116,9 @@ func TestCollectAttachments(t *testing.T) { result := e.collectAttachments(content) - // Map is created but empty since file doesn't exist - assert.Empty(t, result) + // File-reference attachments are passed through; the consumer handles validation. + require.Len(t, result, 1) + assert.Equal(t, "/nonexistent/path/file.txt", result[0].FilePath) assert.Nil(t, e.attachments, "attachments should still be cleared") }) @@ -125,7 +128,8 @@ func TestCollectAttachments(t *testing.T) { tmpDir := t.TempDir() ref := "@" + tmpDir // Note: addFileAttachment would normally reject directories, but we test - // collectAttachments directly here - it will fail to read as file + // collectAttachments directly here — directories are passed through + // as file references; the consumer handles validation. e := &editor{attachments: []attachment{{ path: tmpDir, placeholder: ref, @@ -135,12 +139,12 @@ func TestCollectAttachments(t *testing.T) { result := e.collectAttachments(content) - // os.ReadFile on a directory returns an error, so no attachment added - assert.Empty(t, result) + require.Len(t, result, 1) + assert.Equal(t, tmpDir, result[0].FilePath) assert.Nil(t, e.attachments, "attachments should be cleared after collection") }) - t.Run("mixed valid and invalid refs", func(t *testing.T) { + t.Run("mixed valid and invalid file refs", func(t *testing.T) { t.Parallel() tmpDir := t.TempDir() @@ -157,10 +161,10 @@ func TestCollectAttachments(t *testing.T) { result := e.collectAttachments(content) - require.NotNil(t, result) - assert.Equal(t, "valid content", result[validRef]) - _, hasInvalid := result[invalidRef] - assert.False(t, hasInvalid, "invalid ref should not be in map") + // Both file-ref attachments are passed through; consumer validates. + require.Len(t, result, 2) + assert.Equal(t, validFile, result[0].FilePath) + assert.Equal(t, "/nonexistent/file.txt", result[1].FilePath) assert.Nil(t, e.attachments, "attachments should be cleared after collection") }) } @@ -250,12 +254,13 @@ func TestTryAddFileRef(t *testing.T) { require.Len(t, e.attachments, 2) - // Verify both get collected + // Verify both get collected as file references with paths content := "compare @" + completedFile + " with @" + manualFile result := e.collectAttachments(content) - assert.Equal(t, "package completed", result["@"+completedFile]) - assert.Equal(t, "package manual", result["@"+manualFile]) + require.Len(t, result, 2) + assert.Equal(t, completedFile, result[0].FilePath) + assert.Equal(t, manualFile, result[1].FilePath) }) } diff --git a/pkg/tui/components/editor/paste_test.go b/pkg/tui/components/editor/paste_test.go index 6c85c42d1..0ca257e3f 100644 --- a/pkg/tui/components/editor/paste_test.go +++ b/pkg/tui/components/editor/paste_test.go @@ -124,9 +124,10 @@ func TestCollectAttachments_WithPastes(t *testing.T) { input := "Hello " + att.placeholder + " world" result := e.collectAttachments(input) - // Content should be in the attachments map keyed by placeholder - require.NotNil(t, result) - assert.Equal(t, content, result[att.placeholder]) + // Paste content should be in the attachment's inline Content field + require.Len(t, result, 1) + assert.Equal(t, content, result[0].Content) + assert.Empty(t, result[0].FilePath, "paste attachments should not have a file path") assert.NoFileExists(t, att.path, "paste file should be removed after collection") assert.Empty(t, e.attachments, "attachments should be cleared") } diff --git a/pkg/tui/messages/edit.go b/pkg/tui/messages/edit.go index 8ef1128da..4555f14da 100644 --- a/pkg/tui/messages/edit.go +++ b/pkg/tui/messages/edit.go @@ -12,7 +12,7 @@ type BranchFromEditMsg struct { ParentSessionID string BranchAtPosition int Content string - Attachments map[string]string + Attachments []Attachment } // InvalidateStatusBarMsg signals that the statusbar cache should be invalidated. diff --git a/pkg/tui/messages/session.go b/pkg/tui/messages/session.go index 289199ee0..cc316d953 100644 --- a/pkg/tui/messages/session.go +++ b/pkg/tui/messages/session.go @@ -3,6 +3,24 @@ package messages import "github.com/docker/cagent/pkg/session" +// Attachment represents content attached to a message. It is either a reference +// to a file on disk (FilePath is set) or inline content already in memory +// (Content is set, e.g. pasted text). When FilePath is set the consumer reads +// and classifies the file at send time; when only Content is set the consumer +// uses it directly as inline text. This design lets us add binary-file support +// (images, PDFs, …) in the future by extending the struct with a MimeType hint. +type Attachment struct { + // Name is the human-readable label (e.g. "paste-1", "main.go"). + Name string + // FilePath is the resolved, absolute path to a file on disk. + // Empty when the content is supplied inline (paste attachments). + FilePath string + // Content holds the raw text content. Set for paste attachments whose + // backing temp file is cleaned up before the message reaches the app layer. + // Empty for file-reference attachments that are read from disk. + Content string +} + // Session lifecycle messages control session state and persistence. type ( // NewSessionMsg requests creation of a new session. @@ -52,8 +70,8 @@ type ( // SendMsg contains the content sent to the agent. SendMsg struct { - Content string // Full content sent to the agent (with file contents expanded) - Attachments map[string]string // Map of filename to content for attachments + Content string // Full content sent to the agent (with file contents expanded) + Attachments []Attachment // Attached files or inline content (e.g. pastes) } // SendAttachmentMsg is a message for the first message with an attachment. diff --git a/pkg/tui/page/chat/chat.go b/pkg/tui/page/chat/chat.go index 0a3a23da0..7821ae8f6 100644 --- a/pkg/tui/page/chat/chat.go +++ b/pkg/tui/page/chat/chat.go @@ -135,7 +135,7 @@ type Page interface { // queuedMessage represents a message waiting to be sent to the agent type queuedMessage struct { content string - attachments map[string]string + attachments []msgtypes.Attachment } // maxQueuedMessages is the maximum number of messages that can be queued @@ -178,7 +178,7 @@ type chatPage struct { // Editing state for branching sessions editing bool branchAtPosition int - editAttachments map[string]string // Preserved attachments from original message + editAttachments []msgtypes.Attachment // Preserved attachments from original message // Key map keyMap KeyMap @@ -1001,7 +1001,7 @@ func (p *chatPage) handleInlineEditCancelled(msg messages.InlineEditCancelledMsg // extractAttachmentsFromSession extracts attachments from a session message at the given position. // Attachments are stored as text parts in MultiContent with format "Contents of : ". // TODO(krisetto): meh we can store and retrieve attachments better in the session itself -func (p *chatPage) extractAttachmentsFromSession(position int) map[string]string { +func (p *chatPage) extractAttachmentsFromSession(position int) []msgtypes.Attachment { sess := p.app.Session() if sess == nil || position < 0 || position >= len(sess.Messages) { return nil @@ -1018,7 +1018,7 @@ func (p *chatPage) extractAttachmentsFromSession(position int) map[string]string return nil } - attachments := make(map[string]string) + var attachments []msgtypes.Attachment const prefix = "Contents of " // Skip the first part (main text content), look for attachment parts @@ -1038,15 +1038,15 @@ func (p *chatPage) extractAttachmentsFromSession(position int) map[string]string continue } filename := rest[:colonIdx] - dataURL := rest[colonIdx+2:] - if filename != "" && dataURL != "" { - attachments[filename] = dataURL + content := rest[colonIdx+2:] + if filename != "" && content != "" { + attachments = append(attachments, msgtypes.Attachment{ + Name: filename, + Content: content, + }) } } - if len(attachments) == 0 { - return nil - } return attachments }