fix(skills): prevent empty strings overriding optional env vars in node scripts

cmd/internal/config.go

@@ -41,7 +41,9 @@ type Config struct {
 }
 
 type ConfigParser struct {
-	EnvVars map[string]string
+	EnvVars         map[string]string
+	OptionalEnvVars []string
+	requiredEnvVars []string
 }
 
 // parseEnv replaces environment variables ${ENV_NAME} with their values.
@@ -59,6 +61,25 @@ func (p *ConfigParser) parseEnv(input string) (string, error) {
 
 		// extract the variable name
 		variableName := parts[1]
+
+		isOptional := len(parts) >= 4 && parts[2] != ""
+		if isOptional {
+			// Add to optional list only if it hasn't been explicitly required
+			if !slices.Contains(p.requiredEnvVars, variableName) && !slices.Contains(p.OptionalEnvVars, variableName) {
+				p.OptionalEnvVars = append(p.OptionalEnvVars, variableName)
+			}
+		} else {
+			// Mark as required
+			if !slices.Contains(p.requiredEnvVars, variableName) {
+				p.requiredEnvVars = append(p.requiredEnvVars, variableName)
+			}
+
+			// Remove from optional list if it's there
+			if i := slices.Index(p.OptionalEnvVars, variableName); i != -1 {
+				p.OptionalEnvVars = slices.Delete(p.OptionalEnvVars, i, i+1)
+			}
+		}
+
 		if value, found := os.LookupEnv(variableName); found {
 			p.EnvVars[variableName] = value
 			return value

cmd/internal/config_test.go

@@ -38,12 +38,13 @@ import (
 
 func TestParseEnv(t *testing.T) {
 	tcs := []struct {
-		desc      string
-		env       map[string]string
-		in        string
-		want      string
-		err       bool
-		errString string
+		desc         string
+		env          map[string]string
+		in           string
+		want         string
+		err          bool
+		errString    string
+		wantOptional []string
 	}{
 		{
 			desc:      "without default without env",
@@ -61,22 +62,43 @@ func TestParseEnv(t *testing.T) {
 			want: "bar",
 		},
 		{
-			desc: "with empty default",
-			in:   "${FOO:}",
-			want: "",
+			desc:         "with empty default",
+			in:           "${FOO:}",
+			want:         "",
+			wantOptional: []string{"FOO"},
 		},
 		{
-			desc: "with default",
-			in:   "${FOO:bar}",
-			want: "bar",
+			desc:         "with default",
+			in:           "${FOO:bar}",
+			want:         "bar",
+			wantOptional: []string{"FOO"},
 		},
 		{
 			desc: "with default with env",
 			env: map[string]string{
 				"FOO": "hello",
 			},
-			in:   "${FOO:bar}",
-			want: "hello",
+			in:           "${FOO:bar}",
+			want:         "hello",
+			wantOptional: []string{"FOO"},
+		},
+		{
+			desc: "multiple variables",
+			in:   "user: ${USER_NAME:}, password: ${PASSWORD:}, ip: ${IP:public}, region: ${REGION}",
+			env: map[string]string{
+				"REGION": "us-central1",
+			},
+			want:         "user: , password: , ip: public, region: us-central1",
+			wantOptional: []string{"USER_NAME", "PASSWORD", "IP"},
+		},
+		{
+			desc: "variable required in one place and optional in another",
+			in:   "project_req: ${PROJECT_ID}, project_opt: ${PROJECT_ID:default}",
+			env: map[string]string{
+				"PROJECT_ID": "my_project",
+			},
+			want:         "project_req: my_project, project_opt: my_project",
+			wantOptional: []string{}, // Because it was marked required at least once
 		},
 	}
 	for _, tc := range tcs {
@@ -99,6 +121,14 @@ func TestParseEnv(t *testing.T) {
 			if tc.want != got {
 				t.Fatalf("unexpected want: got %s, want %s", got, tc.want)
 			}
+			if len(parser.OptionalEnvVars) != len(tc.wantOptional) {
+				t.Fatalf("OptionalEnvVars length mismatch: got %d, want %d. Got: %v, Want: %v", len(parser.OptionalEnvVars), len(tc.wantOptional), parser.OptionalEnvVars, tc.wantOptional)
+			}
+			for i, v := range parser.OptionalEnvVars {
+				if v != tc.wantOptional[i] {
+					t.Errorf("OptionalEnvVars element %d mismatch: got %q, want %q", i, v, tc.wantOptional[i])
+				}
+			}
 		})
 	}
 }

cmd/internal/skills/command.go

@@ -191,7 +191,7 @@ func run(cmd *skillsCmd, opts *internal.ToolboxOptions) error {
 
 		for _, toolName := range toolNames {
 			// Generate wrapper script in scripts directory
-			scriptContent, err := generateScriptContent(toolName, configArgsStr, cmd.licenseHeader, cmd.invocationMode, cmd.toolboxVersion)
+			scriptContent, err := generateScriptContent(toolName, configArgsStr, cmd.licenseHeader, cmd.invocationMode, cmd.toolboxVersion, parser.OptionalEnvVars)
 			if err != nil {
 				errMsg := fmt.Errorf("error generating script content for %s: %w", toolName, err)
 				opts.Logger.ErrorContext(ctx, errMsg.Error())

cmd/internal/skills/generator.go

@@ -128,7 +128,11 @@ const os = require('os');
 
 const toolName = "{{.Name}}";
 const configArgs = [{{.ConfigArgs}}];
-
+{{if .OptionalVars}}
+const OPTIONAL_VARS_TO_OMIT_IF_EMPTY = [
+{{range .OptionalVars}}    '{{.}}',
+{{end}}];
+{{end}}
 function getEnv() {
     const envPath = path.resolve(__dirname, '../../../.env');
     const env = { ...process.env };
@@ -158,7 +162,13 @@ if (process.env.GEMINI_CLI === '1') {
     env = getEnv();
     userAgent = "skills-geminicli";
 }
-
+{{if .OptionalVars}}
+OPTIONAL_VARS_TO_OMIT_IF_EMPTY.forEach(varName => {
+    if (env[varName] === '') {
+        delete env[varName];
+    }
+});
+{{end}}
 const args = process.argv.slice(2);
 
 {{if eq .InvocationMode "npx"}}
@@ -219,18 +229,20 @@ type scriptData struct {
 	LicenseHeader  string
 	InvocationMode string
 	ToolboxVersion string
+	OptionalVars   []string
 }
 
 // generateScriptContent creates the content for a Node.js wrapper script.
 // This script invokes the toolbox CLI with the appropriate configuration
 // (using a generated config) and arguments to execute the specific tool.
-func generateScriptContent(name string, configArgs string, licenseHeader string, mode string, version string) (string, error) {
+func generateScriptContent(name string, configArgs string, licenseHeader string, mode string, version string, optionalVars []string) (string, error) {
 	data := scriptData{
 		Name:           name,
 		ConfigArgs:     configArgs,
 		LicenseHeader:  licenseHeader,
 		InvocationMode: mode,
 		ToolboxVersion: version,
+		OptionalVars:   optionalVars,
 	}
 
 	tmpl, err := template.New("script").Parse(nodeScriptTemplate)

cmd/internal/skills/generator_test.go

@@ -221,6 +221,7 @@ func TestGenerateScriptContent(t *testing.T) {
 		licenseHeader string
 		mode          string
 		version       string
+		optionalVars  []string
 	}{
 		{
 			name:       "basic script (binary default)",
@@ -252,6 +253,21 @@ func TestGenerateScriptContent(t *testing.T) {
 				"// My License",
 			},
 		},
+		{
+			name:         "script with optional vars",
+			toolName:     "test-tool",
+			configArgs:   `"--prebuilt", "test"`,
+			optionalVars: []string{"OPTIONAL_A", "OPTIONAL_B"},
+			wantContains: []string{
+				"const OPTIONAL_VARS_TO_OMIT_IF_EMPTY = [",
+				"    'OPTIONAL_A',",
+				"    'OPTIONAL_B',",
+				"];",
+				"OPTIONAL_VARS_TO_OMIT_IF_EMPTY.forEach(varName => {",
+				"    if (env[varName] === '') {",
+				"        delete env[varName];",
+			},
+		},
 		{
 			name:       "npx mode script",
 			toolName:   "npx-tool",
@@ -267,7 +283,7 @@ func TestGenerateScriptContent(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := generateScriptContent(tt.toolName, tt.configArgs, tt.licenseHeader, tt.mode, tt.version)
+			got, err := generateScriptContent(tt.toolName, tt.configArgs, tt.licenseHeader, tt.mode, tt.version, tt.optionalVars)
 			if err != nil {
 				t.Fatalf("generateScriptContent() error = %v", err)
 			}