fix(shell): skip env vars with invalid names instead of breaking the shell

internal/devbox/devbox.go

@@ -394,9 +394,9 @@ func (d *Devbox) EnvExports(ctx context.Context, opts devopt.EnvExportsOpts) (st
 	// Use the appropriate export format based on shell type
 	var envStr string
 	if opts.ShellFormat == devopt.ShellFormatNushell {
-		envStr = exportifyNushell(envs)
+		envStr = exportifyNushell(d.stderr, envs)
 	} else {
-		envStr = exportify(envs)
+		envStr = exportify(d.stderr, envs)
 	}
 
 	if opts.RunHooks {

internal/devbox/envvars.go

@@ -4,22 +4,59 @@
 package devbox
 
 import (
+	"fmt"
+	"io"
 	"os"
+	"regexp"
 	"slices"
 	"strings"
 
 	"go.jetify.com/devbox/internal/devbox/envpath"
 	"go.jetify.com/devbox/internal/envir"
+	"go.jetify.com/devbox/internal/ux"
 )
 
 const devboxSetPrefix = "__DEVBOX_SET_"
 
+// envNameRegexp matches a valid POSIX shell environment variable name: it must
+// start with a letter or underscore and contain only letters, digits, and
+// underscores. Names that don't match (e.g. a "//" comment key in devbox.json's
+// env block) can't be exported without producing invalid shell syntax.
+var envNameRegexp = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
+
+func isValidEnvName(name string) bool {
+	return envNameRegexp.MatchString(name)
+}
+
+// warnInvalidEnvNames prints a single warning naming any environment variables
+// that were skipped because they aren't valid shell identifiers. The most common
+// cause is a "//" comment key in a devbox.json env block, which devbox would
+// otherwise emit as `export //=...` and break the entire shell with a cryptic
+// error.
+func warnInvalidEnvNames(w io.Writer, names []string) {
+	if len(names) == 0 {
+		return
+	}
+	quoted := make([]string, len(names))
+	for i, name := range names {
+		quoted[i] = fmt.Sprintf("%q", name)
+	}
+	ux.Fwarningf(
+		w,
+		"Skipping %d environment variable(s) with invalid names: %s.\n"+
+			"Environment variable names must match ^[a-zA-Z_][a-zA-Z0-9_]*$. "+
+			"If these are \"//\" comments in your devbox.json env block, remove or rename them.\n",
+		len(names),
+		strings.Join(quoted, ", "),
+	)
+}
+
 // exportify formats vars as a line-separated string of shell export statements.
 // Each line is of the form `export key="value";` with any special characters in
 // value escaped. This means that the shell will always interpret values as
 // literal strings; no variable expansion or command substitution will take
 // place.
-func exportify(vars map[string]string) string {
+func exportify(w io.Writer, vars map[string]string) string {
 	keys := make([]string, len(vars))
 	i := 0
 	for k := range vars {
@@ -28,6 +65,7 @@ func exportify(vars map[string]string) string {
 	}
 	slices.Sort(keys) // for reproducibility
 
+	var invalidNames []string
 	strb := strings.Builder{}
 	for _, key := range keys {
 		if strings.HasPrefix(key, "BASH_FUNC_") && strings.HasSuffix(key, "%%") {
@@ -41,7 +79,13 @@ func exportify(vars map[string]string) string {
 			strb.WriteString(funcName)
 			strb.WriteString("\n")
 		} else {
-			// Regular variable
+			// Regular variable. Skip names that aren't valid shell
+			// identifiers; exporting them would produce invalid syntax that
+			// breaks the whole shell (e.g. `export //=...`).
+			if !isValidEnvName(key) {
+				invalidNames = append(invalidNames, key)
+				continue
+			}
 			strb.WriteString("export ")
 			strb.WriteString(key)
 			strb.WriteString(`="`)
@@ -57,12 +101,13 @@ func exportify(vars map[string]string) string {
 			strb.WriteString("\";\n")
 		}
 	}
+	warnInvalidEnvNames(w, invalidNames)
 	return strings.TrimSpace(strb.String())
 }
 
 // exportifyNushell formats vars as nushell environment variable assignments.
 // Each line is of the form `$env.KEY = "value"` with special characters escaped.
-func exportifyNushell(vars map[string]string) string {
+func exportifyNushell(w io.Writer, vars map[string]string) string {
 	// Nushell protected environment variables that cannot be set manually
 	// See: https://www.nushell.sh/book/environment.html#automatic-environment-variables
 	protectedVars := map[string]bool{
@@ -82,6 +127,7 @@ func exportifyNushell(vars map[string]string) string {
 	}
 	slices.Sort(keys) // for reproducibility
 
+	var invalidNames []string
 	strb := strings.Builder{}
 	for _, key := range keys {
 		// Skip bash functions for nushell
@@ -94,6 +140,12 @@ func exportifyNushell(vars map[string]string) string {
 			continue
 		}
 
+		// Skip names that aren't valid environment variable identifiers.
+		if !isValidEnvName(key) {
+			invalidNames = append(invalidNames, key)
+			continue
+		}
+
 		// Nushell environment variable syntax: $env.KEY = "value"
 		strb.WriteString("$env.")
 		strb.WriteString(key)
@@ -108,6 +160,7 @@ func exportifyNushell(vars map[string]string) string {
 		}
 		strb.WriteString("\"\n")
 	}
+	warnInvalidEnvNames(w, invalidNames)
 	return strings.TrimSpace(strb.String())
 }
 

internal/devbox/envvars_test.go

@@ -0,0 +1,62 @@
+// Copyright 2024 Jetify Inc. and contributors. All rights reserved.
+// Use of this source code is governed by the license in the LICENSE file.
+
+package devbox
+
+import (
+	"io"
+	"strings"
+	"testing"
+)
+
+func TestIsValidEnvName(t *testing.T) {
+	valid := []string{"FOO", "_foo", "foo_BAR_123", "a", "_"}
+	for _, name := range valid {
+		if !isValidEnvName(name) {
+			t.Errorf("isValidEnvName(%q) = false, want true", name)
+		}
+	}
+
+	invalid := []string{"//", "//ccache", "bad.name", "1leading", "with space", "with-dash", ""}
+	for _, name := range invalid {
+		if isValidEnvName(name) {
+			t.Errorf("isValidEnvName(%q) = true, want false", name)
+		}
+	}
+}
+
+// TestExportifySkipsInvalidNames ensures that env vars whose names aren't valid
+// shell identifiers (e.g. a "//" comment key in devbox.json) are dropped instead
+// of producing invalid shell that breaks the whole shell.
+func TestExportifySkipsInvalidNames(t *testing.T) {
+	got := exportify(io.Discard, map[string]string{
+		"GOOD":     "value",
+		"//":       "comment-as-json-hack",
+		"//ccache": "another comment",
+		"bad.name": "dotted",
+		"1leading": "starts with digit",
+	})
+
+	if !strings.Contains(got, `export GOOD="value";`) {
+		t.Errorf("expected valid var to be exported, got:\n%s", got)
+	}
+	for _, bad := range []string{"//", "//ccache", "bad.name", "1leading"} {
+		if strings.Contains(got, bad) {
+			t.Errorf("expected invalid name %q to be skipped, got:\n%s", bad, got)
+		}
+	}
+}
+
+func TestExportifyNushellSkipsInvalidNames(t *testing.T) {
+	got := exportifyNushell(io.Discard, map[string]string{
+		"GOOD": "value",
+		"//":   "comment",
+	})
+
+	if !strings.Contains(got, `$env.GOOD = "value"`) {
+		t.Errorf("expected valid var to be exported, got:\n%s", got)
+	}
+	if strings.Contains(got, "//") {
+		t.Errorf("expected invalid name to be skipped, got:\n%s", got)
+	}
+}

internal/devbox/shell.go

@@ -338,7 +338,7 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
 		HooksFilePath:      shellgen.ScriptPath(s.projectDir, shellgen.HooksFilename),
 		ShellStartTime:     telemetry.FormatShellStart(s.shellStartTime),
 		HistoryFile:        strings.TrimSpace(s.historyFile),
-		ExportEnv:          exportify(s.env),
+		ExportEnv:          exportify(s.devbox.stderr, s.env),
 		ShellName:          string(s.name),
 		ShellAliases:       s.aliasLines(),
 		RefreshAliasName:   s.devbox.refreshAliasName(),