fix(im): fail closed on media upload errors instead of rewriting content

2026-08-03 08:32:46 +08:00 · 2026-07-17 13:05:38 +08:00
parent ff9a3e7c23
commit 4b0ba66e8f
5 changed files with 175 additions and 47 deletions
--- a/shortcuts/im/coverage_additional_test.go
+++ b/shortcuts/im/coverage_additional_test.go
@@ -17,6 +17,7 @@ import (
 	larkcore "github.com/larksuite/oapi-sdk-go/v3/core"
 	"github.com/spf13/cobra"

+	"github.com/larksuite/cli/errs"
 	"github.com/larksuite/cli/internal/cmdutil"
 )

@@ -98,7 +99,10 @@ func TestReadDurationHelpersInvalid(t *testing.T) {
 }

 func TestResolveMarkdownAsPost(t *testing.T) {
-	got := resolveMarkdownAsPost(context.Background(), nil, "# Title\n## Subtitle\n\nbody")
+	got, err := resolveMarkdownAsPost(context.Background(), nil, "# Title\n## Subtitle\n\nbody")
+	if err != nil {
+		t.Fatalf("resolveMarkdownAsPost() error = %v", err)
+	}
 	if !strings.Contains(got, `"tag":"md"`) {
 		t.Fatalf("resolveMarkdownAsPost() = %q, want post payload", got)
 	}
@@ -110,6 +114,33 @@ func TestResolveMarkdownAsPost(t *testing.T) {
 	}
 }

+// TestResolveMarkdownImageURLsFailureAborts locks the governance contract for
+// markdown images that fail to resolve: the whole send aborts — the image is
+// never silently stripped, because the user approved a draft that includes it.
+func TestResolveMarkdownImageURLsFailureAborts(t *testing.T) {
+	runtime := newBotShortcutRuntime(t, shortcutRoundTripFunc(func(req *http.Request) (*http.Response, error) {
+		return nil, fmt.Errorf("unexpected request: %s", req.URL.String())
+	}))
+
+	md := "before ![diagram](http://127.0.0.1/pic.png) after"
+	got, err := resolveMarkdownImageURLs(context.Background(), runtime, md)
+	if err == nil {
+		t.Fatalf("resolveMarkdownImageURLs() = (%q, nil), want hard error instead of stripping the image", got)
+	}
+	if got != "" {
+		t.Fatalf("resolveMarkdownImageURLs() returned content %q alongside error", got)
+	}
+	problem, ok := errs.ProblemOf(err)
+	if !ok {
+		t.Fatalf("resolveMarkdownImageURLs() error is not a typed Problem: %v", err)
+	}
+	for _, want := range []string{"nothing was sent", "approval"} {
+		if !strings.Contains(problem.Hint, want) {
+			t.Fatalf("resolveMarkdownImageURLs() hint = %q, want it to contain %q", problem.Hint, want)
+		}
+	}
+}
+
 func TestValidateContentFlags(t *testing.T) {
 	tests := []struct {
 		name       string
@@ -496,7 +527,11 @@ func TestParseMediaDurationSuccess(t *testing.T) {
 	})
 }

-func TestResolveMediaContentURLFallback(t *testing.T) {
+// TestResolveMediaContentURLUploadFailure locks the governance contract for
+// URL media whose upload fails: the send must hard-fail with a re-approval
+// hint — never downgrade to a "[... upload failed, sending link]" text the
+// user never approved (the pre-governance fallback behavior).
+func TestResolveMediaContentURLUploadFailure(t *testing.T) {
 	runtime := newBotShortcutRuntime(t, shortcutRoundTripFunc(func(req *http.Request) (*http.Response, error) {
 		return nil, fmt.Errorf("unexpected request: %s", req.URL.String())
 	}))
@@ -508,26 +543,30 @@ func TestResolveMediaContentURLFallback(t *testing.T) {
 		video      string
 		videoCover string
 		audio      string
-		wantType   string
-		wantText   string
 	}{
-		{name: "image URL fallback", image: "http://127.0.0.1/image.png", wantType: "text", wantText: "[image upload failed, sending link] http://127.0.0.1/image.png"},
-		{name: "file URL fallback", file: "http://127.0.0.1/report.pdf", wantType: "text", wantText: "[file upload failed, sending link] http://127.0.0.1/report.pdf"},
-		{name: "video URL fallback", video: "http://127.0.0.1/video.mp4", videoCover: "img_cover_x", wantType: "text", wantText: "[video upload failed, sending link] http://127.0.0.1/video.mp4"},
-		{name: "audio URL fallback", audio: "http://127.0.0.1/audio.ogg", wantType: "text", wantText: "[audio upload failed, sending link] http://127.0.0.1/audio.ogg"},
+		{name: "image URL upload failure", image: "https://mock.example.com/image.png"},
+		{name: "file URL upload failure", file: "https://mock.example.com/report.pdf"},
+		{name: "video URL upload failure", video: "https://mock.example.com/video.mp4", videoCover: "img_cover_x"},
+		{name: "audio URL upload failure", audio: "https://mock.example.com/audio.ogg"},
 	}

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			gotType, gotContent, err := resolveMediaContent(context.Background(), runtime, "", tt.image, tt.file, tt.video, tt.videoCover, tt.audio)
-			if err != nil {
-				t.Fatalf("resolveMediaContent() error = %v", err)
+			if err == nil {
+				t.Fatalf("resolveMediaContent() = (%q, %q, nil), want hard error instead of text fallback", gotType, gotContent)
 			}
-			if gotType != tt.wantType {
-				t.Fatalf("resolveMediaContent() type = %q, want %q", gotType, tt.wantType)
+			if gotType != "" || gotContent != "" {
+				t.Fatalf("resolveMediaContent() returned content (%q, %q) alongside error", gotType, gotContent)
 			}
-			if !strings.Contains(gotContent, tt.wantText) {
-				t.Fatalf("resolveMediaContent() content = %q, want substring %q", gotContent, tt.wantText)
+			problem, ok := errs.ProblemOf(err)
+			if !ok {
+				t.Fatalf("resolveMediaContent() error is not a typed Problem: %v", err)
+			}
+			for _, want := range []string{"nothing was sent", "--text", "approval"} {
+				if !strings.Contains(problem.Hint, want) {
+					t.Fatalf("resolveMediaContent() hint = %q, want it to contain %q (explicit re-approval path)", problem.Hint, want)
+				}
 			}
 		})
 	}
--- a/shortcuts/im/helpers.go
+++ b/shortcuts/im/helpers.go
@@ -400,14 +400,29 @@ func resolveVideoContent(ctx context.Context, runtime *common.RuntimeContext, vi
 	return "media", string(jsonBytes), nil
 }

-// mediaFallbackOrError returns a text fallback for URL inputs when upload fails,
-// or a hard error for local file inputs.
+// mediaUploadFallbackHint is the recovery path for a failed URL-media upload.
+// The CLI must never rewrite approved content on its own, so the degraded
+// form (a plain text link) is only reachable through explicit re-approval.
+const mediaUploadFallbackHint = "nothing was sent — to fall back to sending the link as plain text, show the user the degraded content and, after their approval, re-send it explicitly with --text"
+
+// mediaFallbackOrError returns a hard error when a media upload fails.
+// A failed URL upload used to downgrade to a "[... upload failed, sending
+// link]" text message, which sent the recipient wording the user never saw
+// or approved. Now nothing is sent; for URL inputs the hint points at the
+// explicit re-approval path. An already-typed cause keeps its classification
+// (and its own hint, when it has one).
 func mediaFallbackOrError(originalValue, mediaType string, uploadErr error) (string, string, error) {
 	if isURL(originalValue) {
-		// Fallback: send URL as text link instead of failing.
-		fallbackText := fmt.Sprintf("[%s upload failed, sending link] %s", mediaType, originalValue)
-		jsonBytes, _ := json.Marshal(map[string]string{"text": fallbackText})
-		return "text", string(jsonBytes), nil
+		if p, ok := errs.ProblemOf(uploadErr); ok {
+			if p.Hint == "" {
+				p.Hint = mediaUploadFallbackHint
+			}
+			return "", "", uploadErr
+		}
+		return "", "", errs.NewNetworkError(errs.SubtypeNetworkTransport,
+			"%s upload failed for %s; nothing was sent", mediaType, sanitizeURLForDisplay(originalValue)).
+			WithCause(uploadErr).
+			WithHint("%s", mediaUploadFallbackHint)
 	}
 	return "", "", wrapIMNetworkErr(uploadErr, "%s upload failed", mediaType)
 }
@@ -928,20 +943,29 @@ func wrapMarkdownAsPostForDryRun(markdown string) (content, desc string) {

 // resolveMarkdownAsPost resolves image URLs in markdown, applies style optimization,
 // and wraps as post format JSON. Used by Execute (makes network calls).
-func resolveMarkdownAsPost(ctx context.Context, runtime *common.RuntimeContext, markdown string) string {
-	resolved := resolveMarkdownImageURLs(ctx, runtime, markdown)
+func resolveMarkdownAsPost(ctx context.Context, runtime *common.RuntimeContext, markdown string) (string, error) {
+	resolved, err := resolveMarkdownImageURLs(ctx, runtime, markdown)
+	if err != nil {
+		return "", err
+	}
 	optimized := optimizeMarkdownStyle(resolved)
 	inner, _ := json.Marshal(optimized)
-	return `{"zh_cn":{"content":[[{"tag":"md","text":` + string(inner) + `}]]}}`
+	return `{"zh_cn":{"content":[[{"tag":"md","text":` + string(inner) + `}]]}}`, nil
 }

 // resolveMarkdownImageURLs finds ![alt](https://...) in markdown, downloads each URL,
-// uploads as image, and replaces with ![alt](img_xxx). Failed uploads are stripped.
-func resolveMarkdownImageURLs(ctx context.Context, runtime *common.RuntimeContext, markdown string) string {
+// uploads as image, and replaces with ![alt](img_xxx). A failed download or
+// upload aborts the send: silently stripping the image would deliver content
+// the user never approved (the message they saw included that image).
+func resolveMarkdownImageURLs(ctx context.Context, runtime *common.RuntimeContext, markdown string) (string, error) {
 	if !strings.Contains(markdown, "![") {
-		return markdown
+		return markdown, nil
 	}
-	return reMarkdownImage.ReplaceAllStringFunc(markdown, func(m string) string {
+	var resolveErr error
+	resolved := reMarkdownImage.ReplaceAllStringFunc(markdown, func(m string) string {
+		if resolveErr != nil {
+			return m
+		}
 		sub := reMarkdownImage.FindStringSubmatch(m)
 		if len(sub) < 2 {
 			return m
@@ -950,16 +974,16 @@ func resolveMarkdownImageURLs(ctx context.Context, runtime *common.RuntimeContex

 		rc, _, err := downloadURLToReader(ctx, runtime, imgURL, maxImageUploadSize, "--markdown")
 		if err != nil {
-			fmt.Fprintf(runtime.IO().ErrOut, "warning: failed to download image %s: %v\n", sanitizeURLForDisplay(imgURL), err)
-			return ""
+			resolveErr = markdownImageError(imgURL, "download", err)
+			return m
 		}
 		defer rc.Close()

 		fmt.Fprintf(runtime.IO().ErrOut, "uploading image from URL: %s\n", sanitizeURLForDisplay(imgURL))
 		imgKey, err := uploadImageFromReader(ctx, runtime, rc, "message")
 		if err != nil {
-			fmt.Fprintf(runtime.IO().ErrOut, "warning: failed to upload image %s: %v\n", sanitizeURLForDisplay(imgURL), err)
-			return ""
+			resolveErr = markdownImageError(imgURL, "upload", err)
+			return m
 		}

 		// Reconstruct ![alt](img_xxx)
@@ -971,6 +995,33 @@ func resolveMarkdownImageURLs(ctx context.Context, runtime *common.RuntimeContex
 		}
 		return fmt.Sprintf("![%s](%s)", alt, imgKey)
 	})
+	if resolveErr != nil {
+		return "", resolveErr
+	}
+	return resolved, nil
+}
+
+// markdownImageFallbackHint is the recovery path for a markdown image that
+// could not be resolved: revise the draft explicitly instead of letting the
+// CLI strip the image behind the user's back.
+const markdownImageFallbackHint = "nothing was sent — remove the failing image from the markdown or replace it with a plain link, show the user the revised draft, and re-send after their approval"
+
+// markdownImageError builds the hard error for a markdown image that could
+// not be resolved. Stripping the image and sending the rest is forbidden —
+// that would deliver content differing from what the user approved. An
+// already-typed cause keeps its classification (and its own hint, when it
+// has one).
+func markdownImageError(imgURL, stage string, cause error) error {
+	if p, ok := errs.ProblemOf(cause); ok {
+		if p.Hint == "" {
+			p.Hint = markdownImageFallbackHint
+		}
+		return cause
+	}
+	return errs.NewNetworkError(errs.SubtypeNetworkTransport,
+		"markdown image %s failed for %s; nothing was sent", stage, sanitizeURLForDisplay(imgURL)).
+		WithCause(cause).
+		WithHint("%s", markdownImageFallbackHint)
 }

 // validateContentFlags checks mutual exclusion between content flags (text/markdown/content)
--- a/shortcuts/im/helpers_test.go
+++ b/shortcuts/im/helpers_test.go
@@ -438,19 +438,46 @@ func TestFileNameFromURL(t *testing.T) {
 func TestMediaFallbackOrError(t *testing.T) {
 	testErr := errors.New("upload failed")

-	// URL input: should fallback to text
+	// URL input: must hard-fail — never downgrade to a text link the user
+	// never approved. The hint must point at the explicit re-approval path.
 	mt, content, err := mediaFallbackOrError("https://example.com/photo.jpg", "image", testErr)
-	if err != nil {
-		t.Fatalf("mediaFallbackOrError(URL) returned error: %v", err)
+	if err == nil {
+		t.Fatalf("mediaFallbackOrError(URL) = (%q, %q, nil), want hard error", mt, content)
 	}
-	if mt != "text" {
-		t.Fatalf("mediaFallbackOrError(URL) mt = %q, want text", mt)
+	if mt != "" || content != "" {
+		t.Fatalf("mediaFallbackOrError(URL) returned content (%q, %q) alongside error", mt, content)
 	}
-	if !strings.Contains(content, "https://example.com/photo.jpg") {
-		t.Fatalf("mediaFallbackOrError(URL) content missing URL: %s", content)
+	problem, ok := errs.ProblemOf(err)
+	if !ok {
+		t.Fatalf("mediaFallbackOrError(URL) error is not a typed Problem: %v", err)
+	}
+	if !strings.Contains(problem.Message, "nothing was sent") {
+		t.Fatalf("mediaFallbackOrError(URL) message = %q, want it to state nothing was sent", problem.Message)
+	}
+	if !strings.Contains(problem.Hint, "--text") || !strings.Contains(problem.Hint, "approval") {
+		t.Fatalf("mediaFallbackOrError(URL) hint = %q, want explicit --text re-approval path", problem.Hint)
 	}

-	// Local file input: should return hard error
+	// A cause that is already a typed Problem passes through with its
+	// classification preserved and, lacking its own hint, gains the
+	// governance re-approval hint.
+	typedCause := errs.NewPermissionError(errs.SubtypePermissionDenied, "missing scope")
+	_, _, err = mediaFallbackOrError("https://example.com/photo.jpg", "image", typedCause)
+	if err != error(typedCause) {
+		t.Fatalf("mediaFallbackOrError(URL, typed cause) = %v, want the cause passed through", err)
+	}
+	if p, _ := errs.ProblemOf(err); p == nil || !strings.Contains(p.Hint, "--text") {
+		t.Fatalf("mediaFallbackOrError(URL, typed cause) hint = %v, want governance hint attached", p)
+	}
+
+	// A typed cause that already carries a hint keeps it.
+	hinted := errs.NewPermissionError(errs.SubtypePermissionDenied, "missing scope").WithHint("run auth login")
+	_, _, err = mediaFallbackOrError("https://example.com/photo.jpg", "image", hinted)
+	if p, _ := errs.ProblemOf(err); p == nil || p.Hint != "run auth login" {
+		t.Fatalf("mediaFallbackOrError(URL, hinted cause) hint = %v, want original hint kept", p)
+	}
+
+	// Local file input: hard error as before.
 	_, _, err = mediaFallbackOrError("./local.jpg", "image", testErr)
 	if err == nil {
 		t.Fatal("mediaFallbackOrError(local) should return error")
@@ -459,7 +486,10 @@ func TestMediaFallbackOrError(t *testing.T) {

 func TestResolveMarkdownImageURLs_NoImages(t *testing.T) {
 	input := "just text, no images"
-	got := resolveMarkdownImageURLs(context.Background(), nil, input)
+	got, err := resolveMarkdownImageURLs(context.Background(), nil, input)
+	if err != nil {
+		t.Fatalf("resolveMarkdownImageURLs(no images) returned error: %v", err)
+	}
 	if got != input {
 		t.Fatalf("resolveMarkdownImageURLs(no images) changed text: %q", got)
 	}
--- a/shortcuts/im/im_messages_reply.go
+++ b/shortcuts/im/im_messages_reply.go
@@ -38,8 +38,8 @@ var ImMessagesReply = common.Shortcut{
 		{Name: "idempotency-key", Desc: "idempotency key, max 50 characters (prevents duplicate sends)"},
 	},
 	Tips: []string{
-		`Example: lark-cli im +messages-reply --message-id <message_id> --text "reply"`,
-		`Example: lark-cli im +messages-reply --message-id <message_id> --text "reply" --reply-in-thread`,
+		`Example: lark-cli im +messages-reply --message-id <message_id> --text "reply" --as bot`,
+		`Example: lark-cli im +messages-reply --message-id <message_id> --text "reply" --reply-in-thread --as bot`,
 	},
 	DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI {
 		messageId := runtime.Str("message-id")
@@ -155,7 +155,11 @@ var ImMessagesReply = common.Shortcut{
 		}

 		if markdown != "" {
-			msgType, content = "post", resolveMarkdownAsPost(ctx, runtime, markdown)
+			post, err := resolveMarkdownAsPost(ctx, runtime, markdown)
+			if err != nil {
+				return err
+			}
+			msgType, content = "post", post
 		} else if mt, c, err := resolveMediaContent(ctx, runtime, text, imageVal, fileVal, videoVal, videoCoverVal, audioVal); err != nil {
 			return err
 		} else if mt != "" {
--- a/shortcuts/im/im_messages_send.go
+++ b/shortcuts/im/im_messages_send.go
@@ -40,9 +40,9 @@ var ImMessagesSend = common.Shortcut{
 		{Name: "audio", Desc: audioMessageInputDesc},
 	},
 	Tips: []string{
-		`Example: lark-cli im +messages-send --chat-id <chat_id> --text "hello"`,
-		`Example: lark-cli im +messages-send --user-id <open_id> --text "hello"`,
-		`Example: lark-cli im +messages-send --chat-id <chat_id> --markdown "## update"`,
+		`Example: lark-cli im +messages-send --chat-id <chat_id> --text "hello" --as bot`,
+		`Example: lark-cli im +messages-send --user-id <open_id> --text "hello" --as bot`,
+		`Example: lark-cli im +messages-send --chat-id <chat_id> --markdown "## update" --as bot`,
 	},
 	DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI {
 		chatFlag := runtime.Str("chat-id")
@@ -177,7 +177,11 @@ var ImMessagesSend = common.Shortcut{
 		}
 		// Resolve content type
 		if markdown != "" {
-			msgType, content = "post", resolveMarkdownAsPost(ctx, runtime, markdown)
+			post, err := resolveMarkdownAsPost(ctx, runtime, markdown)
+			if err != nil {
+				return err
+			}
+			msgType, content = "post", post
 		} else if mt, c, err := resolveMediaContent(ctx, runtime, text, imageVal, fileVal, videoVal, videoCoverVal, audioVal); err != nil {
 			return err
 		} else if mt != "" {