Address GitHub Copilot PR review comments on #672

- pkg/cargo/kilnfile.go: UpdateBOSHReleaseTarballLockWithName now forces
  lock.Name to match the name argument (and rejects an empty name),
  instead of trusting the caller's lock.Name — a mismatch could silently
  insert/rename the wrong entry.
- internal/commands/carvel_helpers.go: downloadCarvelRelease reuses
  cargo.KilnfileLock.FindBOSHReleaseWithName instead of re-implementing
  the same linear scan. writeStandardKilnfileLock only tolerates a
  missing lockfile (os.IsNotExist) now — a permissions/IO error used to
  be silently treated the same as "no lockfile", risking overwriting one
  we simply failed to read. It also reuses
  UpdateBOSHReleaseTarballLockWithName instead of duplicating the upsert
  loop.
- internal/commands/carvel_upload.go: no longer misattributes a
  Kilnfile-side load failure (e.g. an unresolved variable(...) call) to
  Kilnfile.lock — checks the Kilnfile in isolation first.
- internal/commands/carvel_bake.go: corrected a log message that said
  "No Kilnfile/Kilnfile.lock found" when only the lockfile's absence is
  actually confirmed.
- internal/carvel/baker.go:
  - generateRuntimeConfigs's hook validation error is now mode-specific
    (pre-install/post-install), matching generateBoshReleaseDir's error,
    via a shared hookModeGroups() helper.
  - The synthesized hook script no longer execs the raw base.yml command
    string unquoted. shellQuoteCommand splits it into words and
    single-quotes each one before rejoining, so shell metacharacters in a
    hook command can't be interpreted by the generated script.
- internal/acceptance/carvel/carvel_workflow_test.go: Step 3 was missing
  --from-lockfile, so it silently baked from source instead of
  exercising the Artifactory-download path it's supposed to test; its
  GetCount assertion was commented out (referencing an undeclared
  variable) rather than fixed. Added --from-lockfile and restored a
  working assertion.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Author: ichandrabhatta

internal/acceptance/carvel/carvel_workflow_test.go

@@ -304,13 +304,18 @@ var _ = Describe("carvel full workflow", Ordered, func() {
 	It("Step 3: bakes a tile using Kilnfile.lock (CI path with Artifactory download)", func() {
 		outputFile := filepath.Join(tmpDir, "step3-ci.pivotal")
 
-		_ = art.GetCount()
+		getCountBefore := art.GetCount()
 
 		cmd := exec.Command(pathToMain,
 			append([]string{
 				"carvel", "bake",
 				"--source-directory", inputPath,
 				"--output-file", outputFile,
+				// Without --from-lockfile, `carvel bake` regenerates the
+				// tile's own release from source (same as Step 1) and never
+				// touches Artifactory — it wouldn't exercise the lockfile/CI
+				// path this step is meant to test.
+				"--from-lockfile",
 				"--verbose",
 			}, variableFlags()...)...,
 		)
@@ -320,9 +325,8 @@ var _ = Describe("carvel full workflow", Ordered, func() {
 
 		assertValidTile(outputFile)
 
-		// With the new local cache check, it might not download if it's already in the cache.
-		// Expect(art.GetCount()).To(BeNumerically(">", getCountBefore),
-		// 	"CI bake must download the cached BOSH release from Artifactory")
+		Expect(art.GetCount()).To(BeNumerically(">", getCountBefore),
+			"CI bake must download the cached BOSH release from Artifactory")
 	})
 
 	// -----------------------------------------------------------------------

internal/carvel/baker.go

@@ -260,6 +260,46 @@ func (b *baker) hookJobName(hookName string) string {
 	return prefix + hookName
 }
 
+// shellQuoteCommand splits a hook's declared command on whitespace and
+// individually single-quotes each word before rejoining them, so the
+// generated hook script's `exec <command>` line safely execs the intended
+// binary/args even if a word contains shell metacharacters (;, $(), `, etc.)
+// — accidental or otherwise. base.yml authors still write one plain string
+// (e.g. "/var/vcap/jobs/smoke_tests/bin/run --foo bar"); the shell just never
+// gets a chance to interpret any of it.
+func shellQuoteCommand(command string) string {
+	words := strings.Fields(command)
+	quoted := make([]string, len(words))
+	for i, w := range words {
+		quoted[i] = shellQuoteWord(w)
+	}
+	return strings.Join(quoted, " ")
+}
+
+// shellQuoteWord wraps a single word in POSIX single quotes, escaping any
+// embedded single quote as '"'"' (end quoting, a literal single quote via a
+// double-quoted segment, then resume quoting).
+func shellQuoteWord(word string) string {
+	return "'" + strings.ReplaceAll(word, "'", `'"'"'`) + "'"
+}
+
+// hookModeGroup pairs a hook mode with its declared hooks. Shared by
+// generateBoshReleaseDir (job synthesis) and generateRuntimeConfigs
+// (validation + addon job references) so both report the same
+// mode-specific error for a malformed hook, regardless of which bake path
+// (Bake vs. BakeFromLockfile) is in use.
+type hookModeGroup struct {
+	mode  string
+	hooks []models.HookDeclaration
+}
+
+func (b *baker) hookModeGroups() []hookModeGroup {
+	return []hookModeGroup{
+		{mode: "pre-install", hooks: b.metadata.PreInstallHooks},
+		{mode: "post-install", hooks: b.metadata.PostInstallHooks},
+	}
+}
+
 func (b *baker) GetReleaseVersion() string {
 	return b.releaseVersion
 }
@@ -494,14 +534,7 @@ files:
 		return err
 	}
 
-	type hookModeGroup struct {
-		mode  string
-		hooks []models.HookDeclaration
-	}
-	for _, group := range []hookModeGroup{
-		{mode: "pre-install", hooks: b.metadata.PreInstallHooks},
-		{mode: "post-install", hooks: b.metadata.PostInstallHooks},
-	} {
+	for _, group := range b.hookModeGroups() {
 		for _, hook := range group.hooks {
 			if hook.Name == "" || hook.Command == "" {
 				return fmt.Errorf("%s hook declaration missing name or command", group.mode)
@@ -532,7 +565,7 @@ properties: {}
 			if err = os.MkdirAll(path.Join(dirName, "jobs", jobName, "templates"), 0755); err != nil {
 				return err
 			}
-			templateContent := fmt.Sprintf("#!/bin/bash\nset -euo pipefail\nexec %s\n", hook.Command)
+			templateContent := fmt.Sprintf("#!/bin/bash\nset -euo pipefail\nexec %s\n", shellQuoteCommand(hook.Command))
 			err = os.WriteFile(
 				path.Join(dirName, "jobs", jobName, "templates", templateName),
 				[]byte(templateContent),
@@ -825,14 +858,13 @@ func (b *baker) generateRuntimeConfigs() error {
 	releases := []string{`$( release "` + b.metadata.Name + `" )`}
 	addonJobs := []models.Job{registryDataJob}
 
-	for _, hook := range append(
-		append([]models.HookDeclaration{}, b.metadata.PreInstallHooks...),
-		b.metadata.PostInstallHooks...,
-	) {
-		if hook.Name == "" || hook.Command == "" {
-			return fmt.Errorf("hook declaration missing name or command")
+	for _, group := range b.hookModeGroups() {
+		for _, hook := range group.hooks {
+			if hook.Name == "" || hook.Command == "" {
+				return fmt.Errorf("%s hook declaration missing name or command", group.mode)
+			}
+			addonJobs = append(addonJobs, models.Job{Name: b.hookJobName(hook.Name), Release: b.metadata.Name})
 		}
-		addonJobs = append(addonJobs, models.Job{Name: b.hookJobName(hook.Name), Release: b.metadata.Name})
 	}
 
 	for _, ar := range b.metadata.AdditionalReleases {

internal/carvel/baker_test.go

@@ -9,6 +9,7 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+	"testing"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -766,7 +767,7 @@ consumes:
 				Expect(templatePath).To(BeAnExistingFile())
 				templateData, err := os.ReadFile(templatePath)
 				Expect(err).NotTo(HaveOccurred())
-				Expect(string(templateData)).To(ContainSubstring("exec /var/vcap/jobs/smoke_tests/bin/run"))
+				Expect(string(templateData)).To(ContainSubstring("exec '/var/vcap/jobs/smoke_tests/bin/run'"))
 			})
 
 			It("extends the runtime-config addon with the synthesized job", func() {
@@ -993,7 +994,7 @@ releases:
 
 				subject := NewBaker()
 				err = subject.BakeFromLockfile(inputPath, cargo.Kilnfile{}, cargo.KilnfileLock{}, releaseLock, "/nonexistent/tarball.tgz", BakeOptions{})
-				Expect(err).To(MatchError(ContainSubstring("hook declaration missing name or command")))
+				Expect(err).To(MatchError(ContainSubstring("post-install hook declaration missing name or command")))
 			})
 		})
 	})
@@ -1231,3 +1232,46 @@ releases:
 		})
 	})
 })
+
+func TestShellQuoteCommand(t *testing.T) {
+	for _, tt := range []struct {
+		name    string
+		command string
+		want    string
+	}{
+		{
+			name:    "single word",
+			command: "/var/vcap/jobs/smoke_tests/bin/run",
+			want:    `'/var/vcap/jobs/smoke_tests/bin/run'`,
+		},
+		{
+			name:    "command with args",
+			command: "/var/vcap/jobs/smoke_tests/bin/run --foo bar",
+			want:    `'/var/vcap/jobs/smoke_tests/bin/run' '--foo' 'bar'`,
+		},
+		{
+			name:    "a hostile command is neutralized, not interpreted",
+			command: "/bin/true; rm -rf /",
+			// Everything after the first word becomes literal arguments to
+			// /bin/true — the shell never sees an unquoted ';'.
+			want: `'/bin/true;' 'rm' '-rf' '/'`,
+		},
+		{
+			name:    "command substitution is neutralized",
+			command: "/bin/true $(whoami) `whoami`",
+			want:    "'/bin/true' '$(whoami)' '`whoami`'",
+		},
+		{
+			name:    "embedded single quote is escaped, not closed early",
+			command: `/bin/echo it's`,
+			want:    `'/bin/echo' 'it'"'"'s'`,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			got := shellQuoteCommand(tt.command)
+			if got != tt.want {
+				t.Errorf("shellQuoteCommand(%q) = %q, want %q", tt.command, got, tt.want)
+			}
+		})
+	}
+}

internal/commands/carvel_bake.go

@@ -75,7 +75,11 @@ func (c CarvelBake) Execute(args []string) error {
 	} else if c.Options.FromLockfile {
 		return fmt.Errorf("failed to load Kilnfiles (required for --from-lockfile): %w", loadErr)
 	} else {
-		c.outLogger.Printf("No Kilnfile/Kilnfile.lock found — proceeding without additional_releases support")
+		// lockfilePresent is false here, so the lockfile is confirmed absent
+		// — that's what actually blocks additional_releases/--from-lockfile,
+		// regardless of whether a Kilnfile exists on its own (LoadKilnfiles
+		// always requires both, so we can't tell from loadErr alone).
+		c.outLogger.Printf("No Kilnfile.lock found — proceeding without additional_releases support")
 	}
 
 	if c.Options.FromLockfile {

internal/commands/carvel_helpers.go

@@ -46,16 +46,8 @@ func downloadCarvelRelease(logger *log.Logger, kilnfile cargo.Kilnfile, lock car
 		return "", fmt.Errorf("no releases found in Kilnfile.lock")
 	}
 
-	var releaseLock cargo.BOSHReleaseTarballLock
-	found := false
-	for _, r := range lock.Releases {
-		if r.Name == releaseName {
-			releaseLock = r
-			found = true
-			break
-		}
-	}
-	if !found {
+	releaseLock, err := lock.FindBOSHReleaseWithName(releaseName)
+	if err != nil {
 		return "", fmt.Errorf("release %q not found in Kilnfile.lock", releaseName)
 	}
 
@@ -78,10 +70,19 @@ func downloadCarvelRelease(logger *log.Logger, kilnfile cargo.Kilnfile, lock car
 func writeStandardKilnfileLock(lockfilePath string, releaseName, releaseVersion, remotePath, remoteSourceID, sha1 string) error {
 	var lock cargo.KilnfileLock
 
-	if data, err := os.ReadFile(lockfilePath); err == nil {
+	data, err := os.ReadFile(lockfilePath)
+	switch {
+	case err == nil:
 		if err := yaml.Unmarshal(data, &lock); err != nil {
 			return fmt.Errorf("failed to parse existing Kilnfile.lock: %w", err)
 		}
+	case os.IsNotExist(err):
+		// No existing lockfile to preserve — fine, we're creating one.
+	default:
+		// A permissions/IO error is not the same as "no lockfile exists yet";
+		// treating it that way would silently overwrite a lockfile we simply
+		// failed to read, losing whatever was in it.
+		return fmt.Errorf("failed to read existing Kilnfile.lock: %w", err)
 	}
 
 	newEntry := cargo.BOSHReleaseTarballLock{
@@ -91,17 +92,8 @@ func writeStandardKilnfileLock(lockfilePath string, releaseName, releaseVersion,
 		RemoteSource: remoteSourceID,
 		SHA1:         sha1,
 	}
-
-	found := false
-	for i, r := range lock.Releases {
-		if r.Name == releaseName {
-			lock.Releases[i] = newEntry
-			found = true
-			break
-		}
-	}
-	if !found {
-		lock.Releases = append(lock.Releases, newEntry)
+	if err := (&lock).UpdateBOSHReleaseTarballLockWithName(releaseName, newEntry); err != nil {
+		return fmt.Errorf("failed to update Kilnfile.lock: %w", err)
 	}
 
 	if lock.Stemcell.OS == "" {
@@ -111,7 +103,7 @@ func writeStandardKilnfileLock(lockfilePath string, releaseName, releaseVersion,
 		}
 	}
 
-	data, err := yaml.Marshal(&lock)
+	data, err = yaml.Marshal(&lock)
 	if err != nil {
 		return fmt.Errorf("failed to marshal Kilnfile.lock: %w", err)
 	}

internal/commands/carvel_helpers_test.go

@@ -47,3 +47,33 @@ func TestWriteStandardKilnfileLock_PreservesExisting(t *testing.T) {
 	require.Equal(t, "my-source", updatedLock.Releases[2].RemoteSource)
 	require.Equal(t, "fake-sha1", updatedLock.Releases[2].SHA1)
 }
+
+func TestWriteStandardKilnfileLock_FailsRatherThanOverwriteUnreadableFile(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("running as root bypasses file permission checks")
+	}
+
+	tmpDir := t.TempDir()
+	lockfilePath := filepath.Join(tmpDir, "Kilnfile.lock")
+
+	initialLock := cargo.KilnfileLock{
+		Releases: []cargo.BOSHReleaseTarballLock{
+			{Name: "cf-cli", Version: "1.0.0"},
+		},
+	}
+	data, err := yaml.Marshal(&initialLock)
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(lockfilePath, data, 0644))
+	require.NoError(t, os.Chmod(lockfilePath, 0000))
+	defer func() { _ = os.Chmod(lockfilePath, 0644) }()
+
+	err = writeStandardKilnfileLock(lockfilePath, "ear-k8s-runtime", "3.0.0", "path/to/remote", "my-source", "fake-sha1")
+	require.Error(t, err, "an unreadable (not missing) lockfile must not be silently treated as absent")
+
+	// The file on disk must be untouched — not overwritten with just the
+	// new entry, losing the existing cf-cli entry we couldn't even read.
+	require.NoError(t, os.Chmod(lockfilePath, 0644))
+	onDisk, err := os.ReadFile(lockfilePath)
+	require.NoError(t, err)
+	require.Equal(t, data, onDisk)
+}

internal/commands/carvel_upload.go

@@ -62,13 +62,21 @@ func (c CarvelUpload) Execute(args []string) error {
 	c.Options.Kilnfile = kilnfilePath
 	kilnfile, kilnfileLock, err := c.Options.LoadKilnfiles(nil, nil)
 	if err != nil {
+		// LoadKilnfiles doesn't say which file failed. Check the Kilnfile in
+		// isolation first, so a Kilnfile-side failure (e.g. an unresolved
+		// variable(...) call) isn't misattributed to the lockfile below.
+		kf, kfErr := loadKilnfileOnly(c.Options.Standard)
+		if kfErr != nil {
+			return fmt.Errorf("failed to load Kilnfile: %w", kfErr)
+		}
+		kilnfile = kf
+
 		if _, lockStatErr := os.Stat(c.Options.KilnfileLockPath()); lockStatErr == nil {
+			// The lockfile exists and the Kilnfile loads fine on its own, so
+			// the lockfile itself must be what's failing to load/parse.
 			return fmt.Errorf("failed to load Kilnfile.lock: %w", err)
 		}
-		kilnfile, err = loadKilnfileOnly(c.Options.Standard)
-		if err != nil {
-			return fmt.Errorf("failed to load Kilnfile: %w", err)
-		}
+		// Kilnfile.lock is simply absent — tolerate it, we might be creating it.
 	}
 
 	artConfig, err := findArtifactorySource(kilnfile)

pkg/cargo/kilnfile.go

@@ -46,6 +46,14 @@ func (k KilnfileLock) FindBOSHReleaseWithName(name string) (BOSHReleaseTarballLo
 }
 
 func (k *KilnfileLock) UpdateBOSHReleaseTarballLockWithName(name string, lock BOSHReleaseTarballLock) error {
+	if name == "" {
+		return errors.New("name must not be empty")
+	}
+	// Force the entry's Name to match name, so a caller can't silently
+	// rename/insert a mismatched entry by passing a lock.Name that differs
+	// from the name being updated.
+	lock.Name = name
+
 	for i, r := range k.Releases {
 		if r.Name == name {
 			k.Releases[i] = lock