Visitar URL original
fix(skills): prevent empty strings overriding optional env vars in no… · googleapis/mcp-toolbox@c52adeb · GitHub
Skip to content

Commit c52adeb

Browse files
twishabansalhelloevegemini-code-assist[bot]
authored
fix(skills): prevent empty strings overriding optional env vars in node scripts (#2963)
## Description Fixes an issue where optional environment variables (e.g., `${CLOUD_SQL_POSTGRES_PASSWORD:}`) were being passed as empty strings to the underlying `toolbox` process by the generated Node.js skill scripts. This disrupted fallback logic (like implicit IAM authentication). The `ConfigParser` now tracks optional variables during YAML parsing. The generator uses this list to embed logic in the Node wrapper script to delete the environment variable from the spawned process if it evaluates to `""`. Note: If a variable is marked as required (e.g., `${PROJECT_ID}`) in one source, but optional (e.g., `${PROJECT_ID:}`) in another within the same configuration file, the parser strictly enforces the **required** constraint, ensuring it is not mistakenly omitted when empty. Eg. in the [cloud sql postgres source](https://github.com/googleapis/genai-toolbox/blob/main/internal/prebuiltconfigs/tools/cloud-sql-postgres.yaml), CLOUD_SQL_POSTGRES_PROJECT is treated as required despite being optional for the admin source. ## PR Checklist > Thank you for opening a Pull Request! Before submitting your PR, there are a > few things you can do to make sure it goes smoothly: - [ ] Make sure you reviewed [CONTRIBUTING.md](https://github.com/googleapis/genai-toolbox/blob/main/CONTRIBUTING.md) - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/genai-toolbox/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) - [ ] Make sure to add `!` if this involve a breaking change 🛠️ Fixes #<issue_number_goes_here> --------- Co-authored-by: Haoyu Wang <whaoyu@google.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 parent ea09db9 commit c52adeb

5 files changed

Lines changed: 99 additions & 20 deletions

File tree

‎cmd/internal/config.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ type Config struct {
4141
}
4242

4343
type ConfigParser struct {
44-
EnvVars map[string]string
44+
EnvVars map[string]string
45+
OptionalEnvVars []string
46+
requiredEnvVars []string
4547
}
4648

4749
// parseEnv replaces environment variables ${ENV_NAME} with their values.
@@ -59,6 +61,25 @@ func (p *ConfigParser) parseEnv(input string) (string, error) {
5961

6062
// extract the variable name
6163
variableName := parts[1]
64+
65+
isOptional := len(parts) >= 4 && parts[2] != ""
66+
if isOptional {
67+
// Add to optional list only if it hasn't been explicitly required
68+
if !slices.Contains(p.requiredEnvVars, variableName) && !slices.Contains(p.OptionalEnvVars, variableName) {
69+
p.OptionalEnvVars = append(p.OptionalEnvVars, variableName)
70+
}
71+
} else {
72+
// Mark as required
73+
if !slices.Contains(p.requiredEnvVars, variableName) {
74+
p.requiredEnvVars = append(p.requiredEnvVars, variableName)
75+
}
76+
77+
// Remove from optional list if it's there
78+
if i := slices.Index(p.OptionalEnvVars, variableName); i != -1 {
79+
p.OptionalEnvVars = slices.Delete(p.OptionalEnvVars, i, i+1)
80+
}
81+
}
82+
6283
if value, found := os.LookupEnv(variableName); found {
6384
p.EnvVars[variableName] = value
6485
return value

‎cmd/internal/config_test.go‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,13 @@ import (
3838

3939
func TestParseEnv(t *testing.T) {
4040
tcs := []struct {
41-
desc string
42-
env map[string]string
43-
in string
44-
want string
45-
err bool
46-
errString string
41+
desc string
42+
env map[string]string
43+
in string
44+
want string
45+
err bool
46+
errString string
47+
wantOptional []string
4748
}{
4849
{
4950
desc: "without default without env",
@@ -61,22 +62,43 @@ func TestParseEnv(t *testing.T) {
6162
want: "bar",
6263
},
6364
{
64-
desc: "with empty default",
65-
in: "${FOO:}",
66-
want: "",
65+
desc: "with empty default",
66+
in: "${FOO:}",
67+
want: "",
68+
wantOptional: []string{"FOO"},
6769
},
6870
{
69-
desc: "with default",
70-
in: "${FOO:bar}",
71-
want: "bar",
71+
desc: "with default",
72+
in: "${FOO:bar}",
73+
want: "bar",
74+
wantOptional: []string{"FOO"},
7275
},
7376
{
7477
desc: "with default with env",
7578
env: map[string]string{
7679
"FOO": "hello",
7780
},
78-
in: "${FOO:bar}",
79-
want: "hello",
81+
in: "${FOO:bar}",
82+
want: "hello",
83+
wantOptional: []string{"FOO"},
84+
},
85+
{
86+
desc: "multiple variables",
87+
in: "user: ${USER_NAME:}, password: ${PASSWORD:}, ip: ${IP:public}, region: ${REGION}",
88+
env: map[string]string{
89+
"REGION": "us-central1",
90+
},
91+
want: "user: , password: , ip: public, region: us-central1",
92+
wantOptional: []string{"USER_NAME", "PASSWORD", "IP"},
93+
},
94+
{
95+
desc: "variable required in one place and optional in another",
96+
in: "project_req: ${PROJECT_ID}, project_opt: ${PROJECT_ID:default}",
97+
env: map[string]string{
98+
"PROJECT_ID": "my_project",
99+
},
100+
want: "project_req: my_project, project_opt: my_project",
101+
wantOptional: []string{}, // Because it was marked required at least once
80102
},
81103
}
82104
for _, tc := range tcs {
@@ -99,6 +121,14 @@ func TestParseEnv(t *testing.T) {
99121
if tc.want != got {
100122
t.Fatalf("unexpected want: got %s, want %s", got, tc.want)
101123
}
124+
if len(parser.OptionalEnvVars) != len(tc.wantOptional) {
125+
t.Fatalf("OptionalEnvVars length mismatch: got %d, want %d. Got: %v, Want: %v", len(parser.OptionalEnvVars), len(tc.wantOptional), parser.OptionalEnvVars, tc.wantOptional)
126+
}
127+
for i, v := range parser.OptionalEnvVars {
128+
if v != tc.wantOptional[i] {
129+
t.Errorf("OptionalEnvVars element %d mismatch: got %q, want %q", i, v, tc.wantOptional[i])
130+
}
131+
}
102132
})
103133
}
104134
}

‎cmd/internal/skills/command.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ func run(cmd *skillsCmd, opts *internal.ToolboxOptions) error {
191191

192192
for _, toolName := range toolNames {
193193
// Generate wrapper script in scripts directory
194-
scriptContent, err := generateScriptContent(toolName, configArgsStr, cmd.licenseHeader, cmd.invocationMode, cmd.toolboxVersion)
194+
scriptContent, err := generateScriptContent(toolName, configArgsStr, cmd.licenseHeader, cmd.invocationMode, cmd.toolboxVersion, parser.OptionalEnvVars)
195195
if err != nil {
196196
errMsg := fmt.Errorf("error generating script content for %s: %w", toolName, err)
197197
opts.Logger.ErrorContext(ctx, errMsg.Error())

‎cmd/internal/skills/generator.go‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,11 @@ const os = require('os');
128128
129129
const toolName = "{{.Name}}";
130130
const configArgs = [{{.ConfigArgs}}];
131-
131+
{{if .OptionalVars}}
132+
const OPTIONAL_VARS_TO_OMIT_IF_EMPTY = [
133+
{{range .OptionalVars}} '{{.}}',
134+
{{end}}];
135+
{{end}}
132136
function getEnv() {
133137
const envPath = path.resolve(__dirname, '../../../.env');
134138
const env = { ...process.env };
@@ -158,7 +162,13 @@ if (process.env.GEMINI_CLI === '1') {
158162
env = getEnv();
159163
userAgent = "skills-geminicli";
160164
}
161-
165+
{{if .OptionalVars}}
166+
OPTIONAL_VARS_TO_OMIT_IF_EMPTY.forEach(varName => {
167+
if (env[varName] === '') {
168+
delete env[varName];
169+
}
170+
});
171+
{{end}}
162172
const args = process.argv.slice(2);
163173
164174
{{if eq .InvocationMode "npx"}}
@@ -219,18 +229,20 @@ type scriptData struct {
219229
LicenseHeader string
220230
InvocationMode string
221231
ToolboxVersion string
232+
OptionalVars []string
222233
}
223234

224235
// generateScriptContent creates the content for a Node.js wrapper script.
225236
// This script invokes the toolbox CLI with the appropriate configuration
226237
// (using a generated config) and arguments to execute the specific tool.
227-
func generateScriptContent(name string, configArgs string, licenseHeader string, mode string, version string) (string, error) {
238+
func generateScriptContent(name string, configArgs string, licenseHeader string, mode string, version string, optionalVars []string) (string, error) {
228239
data := scriptData{
229240
Name: name,
230241
ConfigArgs: configArgs,
231242
LicenseHeader: licenseHeader,
232243
InvocationMode: mode,
233244
ToolboxVersion: version,
245+
OptionalVars: optionalVars,
234246
}
235247

236248
tmpl, err := template.New("script").Parse(nodeScriptTemplate)

‎cmd/internal/skills/generator_test.go‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ func TestGenerateScriptContent(t *testing.T) {
221221
licenseHeader string
222222
mode string
223223
version string
224+
optionalVars []string
224225
}{
225226
{
226227
name: "basic script (binary default)",
@@ -252,6 +253,21 @@ func TestGenerateScriptContent(t *testing.T) {
252253
"// My License",
253254
},
254255
},
256+
{
257+
name: "script with optional vars",
258+
toolName: "test-tool",
259+
configArgs: `"--prebuilt", "test"`,
260+
optionalVars: []string{"OPTIONAL_A", "OPTIONAL_B"},
261+
wantContains: []string{
262+
"const OPTIONAL_VARS_TO_OMIT_IF_EMPTY = [",
263+
" 'OPTIONAL_A',",
264+
" 'OPTIONAL_B',",
265+
"];",
266+
"OPTIONAL_VARS_TO_OMIT_IF_EMPTY.forEach(varName => {",
267+
" if (env[varName] === '') {",
268+
" delete env[varName];",
269+
},
270+
},
255271
{
256272
name: "npx mode script",
257273
toolName: "npx-tool",
@@ -267,7 +283,7 @@ func TestGenerateScriptContent(t *testing.T) {
267283

268284
for _, tt := range tests {
269285
t.Run(tt.name, func(t *testing.T) {
270-
got, err := generateScriptContent(tt.toolName, tt.configArgs, tt.licenseHeader, tt.mode, tt.version)
286+
got, err := generateScriptContent(tt.toolName, tt.configArgs, tt.licenseHeader, tt.mode, tt.version, tt.optionalVars)
271287
if err != nil {
272288
t.Fatalf("generateScriptContent() error = %v", err)
273289
}

0 commit comments

Comments
 (0)