Merge pull request #77 from dj-oyu/fix/send-file-url-persist-temp

fix: keep URL-downloaded temp file alive for MediaStore resolution

pkg/tools/send_file.go

@@ -21,11 +21,17 @@ import (
 )
 
 const (
-	sendFileTempDir       = "/tmp/picoclaw-sendfile"
 	sendFileTempMaxAge    = 1 * time.Hour
 	sendFileDownloadLimit = 50 * 1024 * 1024 // 50 MB download limit
 )
 
+// sendFileTempDir returns the directory for URL-downloaded temp files.
+// Uses the shared media temp directory so that MediaStore can resolve paths
+// after Execute returns (MediaStore only stores a path reference, not a copy).
+func sendFileTempDir() string {
+	return filepath.Join(media.TempDir(), "sendfile")
+}
+
 // SendFileTool allows the LLM to send a local file (image, document, etc.)
 // to the user on the current chat channel via the MediaStore pipeline.
 type SendFileTool struct {
@@ -118,15 +124,17 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe
 
 	// Handle URL downloads
 	var resolved string
-	var tempFile string // non-empty when we downloaded a URL
+	var isURL bool
 	if isHTTPURL(path) {
 		var err error
 		resolved, err = downloadToTemp(ctx, path)
 		if err != nil {
 			return ErrorResult(fmt.Sprintf("download failed: %v", err))
 		}
-		tempFile = resolved
+		isURL = true
-		defer os.Remove(tempFile)
+		// Do NOT delete the temp file here — MediaStore only stores a path
+		// reference, so the file must survive until the channel sends it.
+		// CleanupTempFiles() handles stale files periodically.
 	} else {
 		var err error
 		resolved, err = validatePathWithAllowPaths(path, t.workspace, t.restrict, t.allowPaths)
@@ -151,7 +159,7 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe
 
 	filename, _ := args["filename"].(string)
 	if filename == "" {
-		if tempFile != "" {
+		if isURL {
 			filename = filenameFromURL(path)
 		} else {
 			filename = filepath.Base(resolved)
@@ -178,10 +186,12 @@ func isHTTPURL(path string) bool {
 	return strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://")
 }
 
-// downloadToTemp downloads a URL to a temporary file under sendFileTempDir.
+// downloadToTemp downloads a URL to a temporary file under sendFileTempDir().
-// The caller is responsible for removing the returned file.
+// The file must persist until the channel has sent it; cleanup is handled by
+// CleanupTempFiles (periodic) rather than the caller.
 func downloadToTemp(ctx context.Context, rawURL string) (string, error) {
-	if err := os.MkdirAll(sendFileTempDir, 0o700); err != nil {
+	dir := sendFileTempDir()
+	if err := os.MkdirAll(dir, 0o700); err != nil {
 		return "", fmt.Errorf("create temp dir: %w", err)
 	}
 
@@ -206,7 +216,7 @@ func downloadToTemp(ctx context.Context, rawURL string) (string, error) {
 		return "", err
 	}
 	ext := extFromURL(rawURL)
-	tmpPath := filepath.Join(sendFileTempDir, fmt.Sprintf("dl_%x%s", randBytes, ext))
+	tmpPath := filepath.Join(dir, fmt.Sprintf("dl_%x%s", randBytes, ext))
 
 	f, err := os.OpenFile(tmpPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0o600)
 	if err != nil {
@@ -254,7 +264,8 @@ func filenameFromURL(rawURL string) string {
 // CleanupTempFiles removes old temp files from the sendfile temp directory.
 // Files older than sendFileTempMaxAge are deleted.
 func CleanupTempFiles() {
-	entries, err := os.ReadDir(sendFileTempDir)
+	dir := sendFileTempDir()
+	entries, err := os.ReadDir(dir)
 	if err != nil {
 		return
 	}
@@ -268,7 +279,7 @@ func CleanupTempFiles() {
 			continue
 		}
 		if info.ModTime().Before(cutoff) {
-			os.Remove(filepath.Join(sendFileTempDir, e.Name()))
+			os.Remove(filepath.Join(dir, e.Name()))
 		}
 	}
 }

pkg/tools/send_file_test.go

@@ -239,7 +239,7 @@ func TestSendFileTool_URLDownloadHTTPError(t *testing.T) {
 	}
 }
 
-func TestSendFileTool_URLTempFileCleanedUp(t *testing.T) {
+func TestSendFileTool_URLTempFilePersistsForMediaStore(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Write([]byte("data"))
 	}))
@@ -249,31 +249,32 @@ func TestSendFileTool_URLTempFileCleanedUp(t *testing.T) {
 	tool := NewSendFileTool(t.TempDir(), false, 0, store)
 	tool.SetContext("telegram", "chat123")
 
-	// Execute should download and clean up temp file
+	result := tool.Execute(context.Background(), map[string]any{
-	tool.Execute(context.Background(), map[string]any{
 		"path": srv.URL + "/photo.jpg",
 	})
-
+	if result.IsError {
-	// Verify no temp files remain with dl_ prefix in the temp dir
+		t.Fatalf("unexpected error: %s", result.ForLLM)
-	entries, _ := os.ReadDir(sendFileTempDir)
-	for _, e := range entries {
-		if strings.HasPrefix(e.Name(), "dl_") {
-			info, _ := e.Info()
-			// Only flag files created very recently (within this test)
-			if info != nil && time.Since(info.ModTime()) < 5*time.Second {
-				t.Errorf("temp file not cleaned up: %s", e.Name())
-			}
-		}
 	}
+
+	// The temp file must still exist so MediaStore can resolve and read it
+	ref := result.Media[0]
+	resolved, err := store.Resolve(ref)
+	if err != nil {
+		t.Fatalf("Resolve failed: %v", err)
+	}
+	if _, err := os.Stat(resolved); err != nil {
+		t.Errorf("temp file should persist for MediaStore, but got: %v", err)
+	}
+	t.Cleanup(func() { os.Remove(resolved) })
 }
 
 func TestCleanupTempFiles(t *testing.T) {
-	if err := os.MkdirAll(sendFileTempDir, 0o700); err != nil {
+	if err := os.MkdirAll(sendFileTempDir(), 0o700); err != nil {
 		t.Fatal(err)
 	}
 
 	// Create an "old" temp file
-	oldFile := filepath.Join(sendFileTempDir, "dl_test_old.tmp")
+	oldFile := filepath.Join(sendFileTempDir(), "dl_test_old.tmp")
 	if err := os.WriteFile(oldFile, []byte("old"), 0o600); err != nil {
 		t.Fatal(err)
 	}
@@ -282,7 +283,7 @@ func TestCleanupTempFiles(t *testing.T) {
 	os.Chtimes(oldFile, past, past)
 
 	// Create a "new" temp file
-	newFile := filepath.Join(sendFileTempDir, "dl_test_new.tmp")
+	newFile := filepath.Join(sendFileTempDir(), "dl_test_new.tmp")
 	if err := os.WriteFile(newFile, []byte("new"), 0o600); err != nil {
 		t.Fatal(err)
 	}