diff --git a/shortcuts/im/coverage_additional_test.go b/shortcuts/im/coverage_additional_test.go index 47e5fe97e..2a1b44112 100644 --- a/shortcuts/im/coverage_additional_test.go +++ b/shortcuts/im/coverage_additional_test.go @@ -602,3 +602,51 @@ func TestMediaBufferReader(t *testing.T) { } } } + +func TestMediaBufferFileName(t *testing.T) { + tests := []struct { + label string + buf mediaBuffer + want string + }{ + {"original URL filename", mediaBuffer{name: "report.pdf", ext: ".pdf"}, "report.pdf"}, + {"name with spaces", mediaBuffer{name: "Q1 report.pdf", ext: ".pdf"}, "Q1 report.pdf"}, + {"download fallback", mediaBuffer{name: "download", ext: ""}, "download"}, + {"ext not leaked into name", mediaBuffer{name: "x", ext: ".mp4"}, "x"}, + } + for _, tt := range tests { + t.Run(tt.label, func(t *testing.T) { + if got := tt.buf.FileName(); got != tt.want { + t.Fatalf("FileName() = %q, want %q", got, tt.want) + } + }) + } +} + +// TestNewMediaBufferFromBytesURLFilename locks in the URL -> mediaBuffer.name +// wiring so a future refactor cannot regress back to the "media." synthetic +// filename that was shipped in 91067ec. +func TestNewMediaBufferFromBytesURLFilename(t *testing.T) { + tests := []struct { + label string + url string + want string + }{ + {"path filename", "http://example.com/report.pdf", "report.pdf"}, + {"filename survives query string", "http://example.com/videos/clip.mp4?token=abc", "clip.mp4"}, + {"percent-encoded spaces decoded", "http://example.com/Q1%20report.pdf", "Q1 report.pdf"}, + {"no path falls back to download", "http://example.com/", "download"}, + {"non-http scheme falls back to download", "ftp://example.com/x.pdf", "download"}, + } + for _, tt := range tests { + t.Run(tt.label, func(t *testing.T) { + mb := newMediaBufferFromBytes([]byte("payload"), ".pdf", tt.url) + if got := mb.FileName(); got != tt.want { + t.Fatalf("FileName() for %q = %q, want %q", tt.url, got, tt.want) + } + if got := mb.FileName(); strings.HasPrefix(got, "media") && tt.want != "media" { + t.Fatalf("regression: FileName() returned synthetic %q for %q", got, tt.url) + } + }) + } +} diff --git a/shortcuts/im/helpers.go b/shortcuts/im/helpers.go index 053b9f98e..6594887bc 100644 --- a/shortcuts/im/helpers.go +++ b/shortcuts/im/helpers.go @@ -584,6 +584,7 @@ func parseMediaDuration(runtime *common.RuntimeContext, filePath, fileType strin type mediaBuffer struct { data []byte ext string // file extension including leading dot, e.g. ".mp4" + name string // original file name extracted from the source URL } // newMediaBuffer downloads URL content into memory via downloadURLToReader. @@ -598,7 +599,14 @@ func newMediaBuffer(ctx context.Context, runtime *common.RuntimeContext, rawURL if err != nil { return nil, fmt.Errorf("download failed: %w", err) } - return &mediaBuffer{data: data, ext: ext}, nil + return newMediaBufferFromBytes(data, ext, rawURL), nil +} + +// newMediaBufferFromBytes builds a mediaBuffer from already-downloaded bytes. +// Split out from newMediaBuffer so the URL-to-filename wiring is testable +// without going through the hardened download transport. +func newMediaBufferFromBytes(data []byte, ext, rawURL string) *mediaBuffer { + return &mediaBuffer{data: data, ext: ext, name: fileNameFromURL(rawURL)} } // Reader returns a new io.Reader over the buffered data. Each call returns a @@ -608,9 +616,9 @@ func (b *mediaBuffer) Reader() io.Reader { return bytes.NewReader(b.data) } -// FileName returns a synthetic file name based on the URL extension. +// FileName returns the original file name extracted from the source URL. func (b *mediaBuffer) FileName() string { - return "media" + b.ext + return b.name } // FileType returns the IM file type detected from the extension. diff --git a/shortcuts/im/helpers_network_test.go b/shortcuts/im/helpers_network_test.go index 77658a934..034393c83 100644 --- a/shortcuts/im/helpers_network_test.go +++ b/shortcuts/im/helpers_network_test.go @@ -892,3 +892,38 @@ func TestResolveLocalMediaFile(t *testing.T) { t.Fatalf("resolveLocalMedia(file) = %q, want %q", got, "file_via_resolve") } } + +// TestUploadFileToIMPreservesLocalFileName locks in that local uploads keep +// the basename of the caller-supplied path as the multipart file_name, so the +// URL-side fix for mediaBuffer cannot silently regress the local branch later. +func TestUploadFileToIMPreservesLocalFileName(t *testing.T) { + var gotBody string + runtime := newBotShortcutRuntime(t, shortcutRoundTripFunc(func(req *http.Request) (*http.Response, error) { + if strings.Contains(req.URL.Path, "/open-apis/im/v1/files") { + body, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + gotBody = string(body) + return shortcutJSONResponse(200, map[string]interface{}{ + "code": 0, + "data": map[string]interface{}{"file_key": "file_uploaded"}, + }), nil + } + return nil, fmt.Errorf("unexpected request: %s", req.URL.String()) + })) + + cmdutil.TestChdir(t, t.TempDir()) + + localName := "Q1-meeting-notes.pdf" + if err := os.WriteFile(localName, []byte("pdfdata"), 0600); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + if _, err := uploadFileToIM(context.Background(), runtime, "./"+localName, "pdf", ""); err != nil { + t.Fatalf("uploadFileToIM() error = %v", err) + } + if !strings.Contains(gotBody, `name="file_name"`) || !strings.Contains(gotBody, localName) { + t.Fatalf("upload body missing local filename %q; got: %q", localName, gotBody) + } +}