fix: always write a nuspec into generated NuGet packages

pkg/cmd/package/nuget/create/create.go

@@ -1,9 +1,12 @@
 package create
 
 import (
+	"bytes"
+	"encoding/xml"
 	"errors"
 	"fmt"
 	"os"
+	"os/user"
 	"path/filepath"
 	"strings"
 	"time"
@@ -27,6 +30,11 @@ const (
 	FlagReleaseNotesFile = "releaseNotesFile"
 )
 
+// DefaultDescription is used when the caller supplies no description. The nuspec
+// schema requires the element, and this is what the flag help has always
+// promised.
+const DefaultDescription = "A deployment package created from files on disk."
+
 type NuPkgCreateFlags struct {
 	Author           *flag.Flag[[]string] // this need to be multiple and default to current user
 	Title            *flag.Flag[string]
@@ -112,23 +120,33 @@ func createRun(cmd *cobra.Command, opts *NuPkgCreateOptions) error {
 		return err
 	}
 
-	nuspecFilePath := ""
-	if shouldGenerateNuSpec(opts) {
-		defer func() {
-			if nuspecFilePath != "" {
-				err := os.Remove(nuspecFilePath)
-				if err != nil {
-					panic(err)
-				}
-			}
-		}()
-		nuspecFilePath, err = GenerateNuSpec(opts)
+	// Every .nupkg needs a manifest: readers locate it by looking for a single
+	// .nuspec in the archive root, so a package without one is not a valid NuGet
+	// package even though it is a perfectly good zip. Generate one unless the
+	// base path already supplies it, in which case that file is authoritative and
+	// must not be touched.
+	nuspecFileName := opts.Id.Value + ".nuspec"
+	suppliedNuSpec, err := hasSuppliedNuSpec(opts.BasePath.Value, nuspecFileName)
+	if err != nil {
+		return err
+	}
+
+	if suppliedNuSpec {
+		pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Using existing nuspec file \"%s\"\n", nuspecFileName)
+	} else {
+		nuspecFilePath, err := GenerateNuSpec(opts)
 		if err != nil {
 			return err
 		}
-		opts.Include.Value = append(opts.Include.Value, opts.Id.Value+".nuspec")
+		defer func() {
+			if err := os.Remove(nuspecFilePath); err != nil {
+				pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Could not remove generated nuspec file \"%s\": %v\n", nuspecFilePath, err)
+			}
+		}()
 	}
 
+	opts.Include.Value = append(opts.Include.Value, nuspecFileName)
+
 	pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Packing \"%s\" version \"%s\"...\n", opts.Id.Value, opts.Version.Value)
 	outFilePath := pack.BuildOutFileName("nupkg", opts.Id.Value, opts.Version.Value)
 
@@ -198,7 +216,7 @@ func PromptMissing(opts *NuPkgCreateOptions) error {
 			if err := opts.Ask(&survey.Input{
 				Message: "Nuspec description",
 				Help:    "The description to include in the Nuspec file.",
-				Default: "A deployment package created from files on disk.",
+				Default: DefaultDescription,
 			}, &opts.Description.Value); err != nil {
 				return err
 			}
@@ -246,15 +264,38 @@ func applyDefaultsToUnspecifiedPackageOptions(opts *NuPkgCreateOptions) error {
 		opts.Include.Value = append(opts.Include.Value, "**")
 	}
 
-	if len(opts.Author.Value) > 0 {
-		if opts.Description.Value == "" {
-			opts.Description.Value = "A deployment package created from files on disk."
-		}
+	// A manifest is generated for every package now, so these two are no longer
+	// only relevant when the caller opted into metadata: the nuspec schema
+	// requires both, and leaving them out produces a package strict readers
+	// reject.
+	if opts.Description.Value == "" {
+		opts.Description.Value = DefaultDescription
+	}
+
+	if util.Empty(opts.Author.Value) {
+		opts.Author.Value = []string{defaultAuthor(opts.Id.Value)}
 	}
 
 	return nil
 }
 
+// defaultAuthor mirrors what the old Octopus CLI did, and what the Author flag
+// has always said it should do. Where the user cannot be determined the package
+// ID stands in: the element is required, and an obvious placeholder beats
+// failing the command over metadata nobody asked for.
+// currentUser is a seam: user lookup fails on some minimal container images, and
+// the fallback needs to be reachable in a test.
+var currentUser = user.Current
+
+func defaultAuthor(fallback string) string {
+	if current, err := currentUser(); err == nil {
+		if name := strings.TrimSpace(current.Username); name != "" {
+			return name
+		}
+	}
+	return fallback
+}
+
 func getReleaseNotesFromFile(filePath string) (string, error) {
 	_, err := os.Stat(filePath)
 	if err != nil {
@@ -269,22 +310,34 @@ func getReleaseNotesFromFile(filePath string) (string, error) {
 	return string(notes), nil
 }
 
-func shouldGenerateNuSpec(opts *NuPkgCreateOptions) bool {
-	return opts.Description.Value != "" ||
-		opts.Title.Value != "" ||
-		opts.ReleaseNotes.Value != "" ||
-		opts.ReleaseNotesFile.Value != "" ||
-		!util.Empty(opts.Author.Value)
+// escapeXML makes a value safe to drop between two tags. Release notes and
+// descriptions are free text, and an unescaped ampersand or angle bracket is
+// enough to make the whole manifest unparseable.
+func escapeXML(value string) string {
+	var buf bytes.Buffer
+	if err := xml.EscapeText(&buf, []byte(value)); err != nil {
+		// EscapeText only fails if the writer fails, and bytes.Buffer does not.
+		return value
+	}
+	return buf.String()
 }
 
-func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) {
-
-	if opts.Description.Value == "" {
-		return "", errors.New("description is required when generating nuspec metadata")
+// hasSuppliedNuSpec reports whether the base path already contains a manifest
+// for this package. One written by hand is the user's own file: it is theirs to
+// keep, and generating over the top of it would both discard their metadata and
+// delete the file on the way out.
+func hasSuppliedNuSpec(basePath string, nuspecFileName string) (bool, error) {
+	_, err := os.Stat(filepath.Join(basePath, nuspecFileName))
+	if err == nil {
+		return true, nil
 	}
-	if len(opts.Author.Value) == 0 {
-		return "", errors.New("at least one author is required when generating nuspec metadata")
+	if os.IsNotExist(err) {
+		return false, nil
 	}
+	return false, err
+}
+
+func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) {
 
 	releaseNotes := opts.ReleaseNotes.Value
 	if opts.ReleaseNotesFile.Value != "" {
@@ -305,15 +358,15 @@ func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) {
 	sb.WriteString(`<?xml version="1.0" encoding="utf-8"?>` + "\n")
 	sb.WriteString(`<package xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd">` + "\n")
 	sb.WriteString("  <metadata>\n")
-	sb.WriteString("    <id>" + opts.Id.Value + "</id>\n")
-	sb.WriteString("    <version>" + opts.Version.Value + "</version>\n")
+	sb.WriteString("    <id>" + escapeXML(opts.Id.Value) + "</id>\n")
+	sb.WriteString("    <version>" + escapeXML(opts.Version.Value) + "</version>\n")
 	if opts.Title.Value != "" {
-		sb.WriteString("    <title>" + opts.Title.Value + "</title>\n")
+		sb.WriteString("    <title>" + escapeXML(opts.Title.Value) + "</title>\n")
 	}
-	sb.WriteString("    <description>" + opts.Description.Value + "</description>\n")
-	sb.WriteString("    <authors>" + strings.Join(opts.Author.Value, ",") + "</authors>\n")
+	sb.WriteString("    <description>" + escapeXML(opts.Description.Value) + "</description>\n")
+	sb.WriteString("    <authors>" + escapeXML(strings.Join(opts.Author.Value, ",")) + "</authors>\n")
 	if releaseNotes != "" {
-		sb.WriteString("    <releaseNotes>" + releaseNotes + "</releaseNotes>\n")
+		sb.WriteString("    <releaseNotes>" + escapeXML(releaseNotes) + "</releaseNotes>\n")
 	}
 	sb.WriteString("  </metadata>\n")
 	sb.WriteString("</package>\n")