diff --git a/cmd/heygen/video_download.go b/cmd/heygen/video_download.go index d852381..aa6fe30 100644 --- a/cmd/heygen/video_download.go +++ b/cmd/heygen/video_download.go @@ -104,7 +104,12 @@ func newVideoDownloadCmd(ctx *cmdContext) *cobra.Command { "path": dest, }) if err != nil { - return clierrors.New(fmt.Sprintf("failed to encode response: %v", err)) + return &clierrors.CLIError{ + Code: "cli_response_encode_error", + Message: fmt.Sprintf("failed to encode response: %v", err), + Hint: "Please report this CLI bug with the command you ran.", + ExitCode: clierrors.ExitGeneral, + } } return ctx.formatter.Data(data, "", nil) @@ -122,7 +127,12 @@ func extractAssetURL(raw json.RawMessage, videoID string, info assetInfo) (strin Data map[string]json.RawMessage `json:"data"` } if err := json.Unmarshal(raw, &resp); err != nil { - return "", clierrors.New("failed to parse video response") + return "", &clierrors.CLIError{ + Code: "cli_response_parse_error", + Message: "failed to parse the video response", + Hint: "The API response could not be parsed. Retry; if it persists, report it (possible CLI/API mismatch).", + ExitCode: clierrors.ExitGeneral, + } } var status string @@ -180,17 +190,46 @@ func assetHint(field string) string { func downloadFile(ctx context.Context, videoURL, dest string) error { req, err := http.NewRequestWithContext(ctx, http.MethodGet, videoURL, nil) if err != nil { - return clierrors.New(fmt.Sprintf("failed to build download request: %v", err)) + return &clierrors.CLIError{ + Code: "cli_response_parse_error", + Message: fmt.Sprintf("the download URL returned by the API is unusable: %v", err), + Hint: "Re-fetch a fresh URL: heygen video get ", + ExitCode: clierrors.ExitGeneral, + } } resp, err := downloadClient.Do(req) if err != nil { - return clierrors.New(fmt.Sprintf("failed to download video: %v", err)) + // network_error is intentionally NOT cli_-prefixed. It is a shared code + // also emitted by the API-executor transport path + // (internal/client/executor.go); kept bare so both sites agree. Do not + // prefix one site without the other. + return &clierrors.CLIError{ + Code: "network_error", + Message: fmt.Sprintf("failed to download video: %v", err), + Hint: "This is usually a temporary network issue. Check your connection and retry.", + ExitCode: clierrors.ExitGeneral, + } } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return clierrors.New(fmt.Sprintf("download failed with HTTP %d", resp.StatusCode)) + // A presigned asset URL that 403s/404s is expired or revoked (client can + // re-fetch); any other status is the asset host (our storage/CDN) failing. + if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { + return &clierrors.CLIError{ + Code: "cli_download_url_expired", + Message: fmt.Sprintf("download link expired or unavailable (HTTP %d)", resp.StatusCode), + Hint: "This download link has expired. Re-fetch a fresh URL: heygen video get ", + ExitCode: clierrors.ExitGeneral, + } + } + return &clierrors.CLIError{ + Code: "cli_download_failed", + Message: fmt.Sprintf("download failed with HTTP %d", resp.StatusCode), + Hint: "The asset host returned an error fetching the file. This is usually transient — retry shortly.", + ExitCode: clierrors.ExitGeneral, + } } // Write to a temp file in the same directory, then rename on success. @@ -198,7 +237,12 @@ func downloadFile(ctx context.Context, videoURL, dest string) error { dir := filepath.Dir(dest) tmp, err := os.CreateTemp(dir, ".heygen-download-*.tmp") if err != nil { - return clierrors.New(fmt.Sprintf("failed to create temp file in %q: %v", dir, err)) + return &clierrors.CLIError{ + Code: "cli_file_io_error", + Message: fmt.Sprintf("failed to create temp file in %q: %v", dir, err), + Hint: "Could not write locally. Check the destination path and free disk space.", + ExitCode: clierrors.ExitGeneral, + } } tmpPath := tmp.Name() @@ -206,11 +250,21 @@ func downloadFile(ctx context.Context, videoURL, dest string) error { closeErr := tmp.Close() if copyErr != nil { _ = os.Remove(tmpPath) - return clierrors.New(fmt.Sprintf("download interrupted: %v", copyErr)) + return &clierrors.CLIError{ + Code: "cli_download_interrupted", + Message: fmt.Sprintf("download interrupted: %v", copyErr), + Hint: "The transfer was cut off. Retry the download. The partial file was cleaned up.", + ExitCode: clierrors.ExitGeneral, + } } if closeErr != nil { _ = os.Remove(tmpPath) - return clierrors.New(fmt.Sprintf("failed to finalize download: %v", closeErr)) + return &clierrors.CLIError{ + Code: "cli_file_io_error", + Message: fmt.Sprintf("failed to finalize download: %v", closeErr), + Hint: "Could not finalize the file locally. Check free disk space.", + ExitCode: clierrors.ExitGeneral, + } } // Atomic rename. On Windows this may fail if dest is open elsewhere; @@ -218,7 +272,12 @@ func downloadFile(ctx context.Context, videoURL, dest string) error { // same directory so this is safe. if err := os.Rename(tmpPath, dest); err != nil { _ = os.Remove(tmpPath) - return clierrors.New(fmt.Sprintf("failed to move download to %q: %v", dest, err)) + return &clierrors.CLIError{ + Code: "cli_file_io_error", + Message: fmt.Sprintf("failed to move download to %q: %v", dest, err), + Hint: "Could not write to the destination path. Check permissions and free disk space.", + ExitCode: clierrors.ExitGeneral, + } } return nil diff --git a/cmd/heygen/video_download_test.go b/cmd/heygen/video_download_test.go index 66c2cd9..cf8abfa 100644 --- a/cmd/heygen/video_download_test.go +++ b/cmd/heygen/video_download_test.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "fmt" "net/http" "net/http/httptest" "os" @@ -405,11 +406,101 @@ func TestVideoDownload_DownloadFails(t *testing.T) { if res.ExitCode != 1 { t.Fatalf("ExitCode = %d, want 1\nstderr: %s", res.ExitCode, res.Stderr) } + if !strings.Contains(res.Stderr, `"code":"cli_download_failed"`) { + t.Fatalf("stderr should carry cli_download_failed (asset-host 5xx):\n%s", res.Stderr) + } if _, err := os.Stat(dest); !os.IsNotExist(err) { t.Fatalf("expected no output file, stat err = %v", err) } } +// A presigned asset URL that 403s or 404s is expired/revoked — a client-side +// cli_download_url_expired, distinct from the asset host failing (cli_download_failed). +func TestVideoDownload_ExpiredURL(t *testing.T) { + for _, status := range []int{http.StatusForbidden, http.StatusNotFound} { + t.Run(fmt.Sprintf("HTTP_%d", status), func(t *testing.T) { + tmpDir := t.TempDir() + dest := filepath.Join(tmpDir, "expired.mp4") + + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/v3/videos/vid_123": + writeJSON(t, w, map[string]any{ + "data": map[string]any{ + "video_url": srv.URL + "/download/expired.mp4", + "status": "completed", + }, + }) + case r.Method == http.MethodGet && r.URL.Path == "/download/expired.mp4": + w.WriteHeader(status) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) + defer srv.Close() + + res := runCommand(t, srv.URL, "test-key", "video", "download", "vid_123", "--output-path", dest) + if res.ExitCode != 1 { + t.Fatalf("ExitCode = %d, want 1\nstderr: %s", res.ExitCode, res.Stderr) + } + if !strings.Contains(res.Stderr, `"code":"cli_download_url_expired"`) { + t.Fatalf("stderr should carry cli_download_url_expired (HTTP %d on presigned URL):\n%s", status, res.Stderr) + } + if !strings.Contains(res.Stderr, "video get") { + t.Fatalf("expired-URL hint should point to re-fetching via video get:\n%s", res.Stderr) + } + }) + } +} + +// The video-get response failing to parse, or yielding an unusable download URL, +// classifies as cli_response_parse_error (not the opaque error code). +func TestVideoDownload_ResponseParseError(t *testing.T) { + t.Run("malformed video-get JSON", func(t *testing.T) { + dest := filepath.Join(t.TempDir(), "x.mp4") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v3/videos/vid_123" { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{ this is not valid json")) + return + } + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + })) + defer srv.Close() + + res := runCommand(t, srv.URL, "test-key", "video", "download", "vid_123", "--output-path", dest) + if res.ExitCode != 1 { + t.Fatalf("ExitCode = %d, want 1\nstderr: %s", res.ExitCode, res.Stderr) + } + if !strings.Contains(res.Stderr, `"code":"cli_response_parse_error"`) { + t.Fatalf("stderr should carry cli_response_parse_error:\n%s", res.Stderr) + } + }) + + t.Run("unusable download URL", func(t *testing.T) { + dest := filepath.Join(t.TempDir(), "x.mp4") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v3/videos/vid_123" { + writeJSON(t, w, map[string]any{ + "data": map[string]any{"video_url": "http://%zz", "status": "completed"}, + }) + return + } + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + })) + defer srv.Close() + + res := runCommand(t, srv.URL, "test-key", "video", "download", "vid_123", "--output-path", dest) + if res.ExitCode != 1 { + t.Fatalf("ExitCode = %d, want 1\nstderr: %s", res.ExitCode, res.Stderr) + } + if !strings.Contains(res.Stderr, `"code":"cli_response_parse_error"`) { + t.Fatalf("stderr should carry cli_response_parse_error for an unusable URL:\n%s", res.Stderr) + } + }) +} + func TestVideoDownload_AuthRequired(t *testing.T) { srv := setupTestServer(t, map[string]testHandler{}) defer srv.Close() @@ -429,3 +520,100 @@ func writeJSON(t *testing.T, w http.ResponseWriter, payload any) { } _, _ = w.Write(raw) } + +// Transport failure reaching the asset host classifies as network_error (the +// grandfathered shared transport code, intentionally unprefixed). +func TestVideoDownload_NetworkError(t *testing.T) { + dead := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + deadURL := dead.URL + dead.Close() // deadURL now refuses connections + + dest := filepath.Join(t.TempDir(), "x.mp4") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v3/videos/vid_123" { + writeJSON(t, w, map[string]any{ + "data": map[string]any{"video_url": deadURL + "/x.mp4", "status": "completed"}, + }) + return + } + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + })) + defer srv.Close() + + res := runCommand(t, srv.URL, "test-key", "video", "download", "vid_123", "--output-path", dest) + if res.ExitCode != 1 { + t.Fatalf("ExitCode = %d, want 1\nstderr: %s", res.ExitCode, res.Stderr) + } + if !strings.Contains(res.Stderr, `"code":"network_error"`) { + t.Fatalf("stderr should carry network_error (transport failure):\n%s", res.Stderr) + } +} + +// A response that promises more bytes than it delivers, then drops, cuts io.Copy +// off mid-stream and classifies as cli_download_interrupted. +func TestVideoDownload_Interrupted(t *testing.T) { + dest := filepath.Join(t.TempDir(), "x.mp4") + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/v3/videos/vid_123": + writeJSON(t, w, map[string]any{ + "data": map[string]any{"video_url": srv.URL + "/download/x.mp4", "status": "completed"}, + }) + case "/download/x.mp4": + hj, ok := w.(http.Hijacker) + if !ok { + t.Fatal("test server does not support hijacking") + } + conn, buf, err := hj.Hijack() + if err != nil { + t.Fatalf("hijack: %v", err) + } + // Promise 1000 bytes, send 7, then drop the connection → the client's + // io.Copy hits an unexpected EOF. + _, _ = buf.WriteString("HTTP/1.1 200 OK\r\nContent-Length: 1000\r\n\r\npartial") + _ = buf.Flush() + _ = conn.Close() + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) + defer srv.Close() + + res := runCommand(t, srv.URL, "test-key", "video", "download", "vid_123", "--output-path", dest) + if res.ExitCode != 1 { + t.Fatalf("ExitCode = %d, want 1\nstderr: %s", res.ExitCode, res.Stderr) + } + if !strings.Contains(res.Stderr, `"code":"cli_download_interrupted"`) { + t.Fatalf("stderr should carry cli_download_interrupted (mid-stream cutoff):\n%s", res.Stderr) + } +} + +// A destination whose parent directory does not exist fails temp-file creation +// and classifies as cli_file_io_error. +func TestVideoDownload_FileIOError(t *testing.T) { + dest := filepath.Join(t.TempDir(), "no-such-dir", "x.mp4") // parent dir absent + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/v3/videos/vid_123": + writeJSON(t, w, map[string]any{ + "data": map[string]any{"video_url": srv.URL + "/download/x.mp4", "status": "completed"}, + }) + case "/download/x.mp4": + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("video-bytes")) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) + defer srv.Close() + + res := runCommand(t, srv.URL, "test-key", "video", "download", "vid_123", "--output-path", dest) + if res.ExitCode != 1 { + t.Fatalf("ExitCode = %d, want 1\nstderr: %s", res.ExitCode, res.Stderr) + } + if !strings.Contains(res.Stderr, `"code":"cli_file_io_error"`) { + t.Fatalf("stderr should carry cli_file_io_error (temp-create in missing dir):\n%s", res.Stderr) + } +}