Skip to content

Commit af9c706

Browse files
peteski22peteski22
authored
Centralize env vars expansion to the point runtime config is loaded, not later (#144)
1 parent e24bfe2 commit af9c706

File tree

5 files changed

+325
-135
lines changed

5 files changed

+325
-135
lines changed

‎internal/context/context.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,13 @@ func (c *ExecutionContextConfig) Upsert(ec ServerExecutionContext) (UpsertResult
162162
return op, nil
163163
}
164164

165-
// loadExecutionContextConfig loads a runtime execution context file from disk, using the specified path.
166-
func loadExecutionContextConfig(path string) (*ExecutionContextConfig, error) { // TODO: unexport
165+
// loadExecutionContextConfig loads a runtime execution context file from disk and expands environment variables.
166+
//
167+
// The function parses the TOML file at the specified path and automatically expands all ${VAR} references
168+
// in both args and env fields using os.ExpandEnv. Non-existent environment variables are expanded to
169+
// empty strings. This ensures that the loaded configuration contains actual values ready for runtime use,
170+
// rather than template strings that require later expansion.
171+
func loadExecutionContextConfig(path string) (*ExecutionContextConfig, error) {
167172
cfg := NewExecutionContextConfig(path)
168173

169174
if _, err := os.Stat(path); err != nil {
@@ -178,9 +183,20 @@ func loadExecutionContextConfig(path string) (*ExecutionContextConfig, error) {
178183
return nil, fmt.Errorf("execution context file '%s' could not be parsed: %w", path, err)
179184
}
180185

181-
// Manually set the name field for each ServerExecutionContext.
186+
// Manually set the name field for each ServerExecutionContext and expand all ${VAR} references.
182187
for name, server := range cfg.Servers {
183188
server.Name = name
189+
190+
// Expand args.
191+
for i, arg := range server.Args {
192+
server.Args[i] = os.ExpandEnv(arg)
193+
}
194+
195+
// Expand env vars.
196+
for k, v := range server.Env {
197+
server.Env[k] = os.ExpandEnv(v)
198+
}
199+
184200
cfg.Servers[name] = server
185201
}
186202

‎internal/context/context_test.go

Lines changed: 301 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,3 +310,304 @@ func TestUpsert(t *testing.T) {
310310
})
311311
}
312312
}
313+
314+
// TestLoadExecutionContextConfig_VariableExpansion tests that ${VAR} references are expanded at load time.
315+
func TestLoadExecutionContextConfig_VariableExpansion(t *testing.T) {
316+
// Note: Cannot use t.Parallel() because subtests use t.Setenv
317+
318+
testCases := []struct {
319+
name string
320+
content string
321+
envVars map[string]string
322+
expected map[string]ServerExecutionContext
323+
}{
324+
{
325+
name: "expand environment variables in env section",
326+
content: `[servers.test-server]
327+
args = ["--port", "8080"]
328+
[servers.test-server.env]
329+
API_KEY = "${TEST_API_KEY}"
330+
DB_URL = "${TEST_DB_URL}"
331+
LITERAL = "no-expansion"`,
332+
envVars: map[string]string{
333+
"TEST_API_KEY": "secret-key-123",
334+
"TEST_DB_URL": "postgres://localhost:5432/test",
335+
},
336+
expected: map[string]ServerExecutionContext{
337+
"test-server": {
338+
Name: "test-server",
339+
Args: []string{"--port", "8080"},
340+
Env: map[string]string{
341+
"API_KEY": "secret-key-123",
342+
"DB_URL": "postgres://localhost:5432/test",
343+
"LITERAL": "no-expansion",
344+
},
345+
},
346+
},
347+
},
348+
{
349+
name: "expand environment variables in args section",
350+
content: `[servers.test-server]
351+
args = ["--token", "${AUTH_TOKEN}", "--config", "${CONFIG_PATH}", "--literal", "unchanged"]
352+
[servers.test-server.env]
353+
DEBUG = "true"`,
354+
envVars: map[string]string{
355+
"AUTH_TOKEN": "bearer-token-456",
356+
"CONFIG_PATH": "/etc/myapp/config.json",
357+
},
358+
expected: map[string]ServerExecutionContext{
359+
"test-server": {
360+
Name: "test-server",
361+
Args: []string{
362+
"--token",
363+
"bearer-token-456",
364+
"--config",
365+
"/etc/myapp/config.json",
366+
"--literal",
367+
"unchanged",
368+
},
369+
Env: map[string]string{
370+
"DEBUG": "true",
371+
},
372+
},
373+
},
374+
},
375+
{
376+
name: "expand variables with KEY=VALUE format in args",
377+
content: `[servers.test-server]
378+
args = ["--api-key=${API_KEY}", "CONFIG_FILE=${CONFIG_FILE}", "--standalone", "${STANDALONE_VAR}"]`,
379+
envVars: map[string]string{
380+
"API_KEY": "key-789",
381+
"CONFIG_FILE": "/path/to/config",
382+
"STANDALONE_VAR": "standalone-value",
383+
},
384+
expected: map[string]ServerExecutionContext{
385+
"test-server": {
386+
Name: "test-server",
387+
Args: []string{
388+
"--api-key=key-789",
389+
"CONFIG_FILE=/path/to/config",
390+
"--standalone",
391+
"standalone-value",
392+
},
393+
Env: nil, // No env section in TOML means nil map
394+
},
395+
},
396+
},
397+
{
398+
name: "non-existent variables expand to empty string",
399+
content: `[servers.test-server]
400+
args = ["--missing", "${NON_EXISTENT_VAR}", "--empty=${ANOTHER_MISSING}"]
401+
[servers.test-server.env]
402+
MISSING_VALUE = "${UNDEFINED_ENV_VAR}"
403+
PRESENT_VALUE = "${DEFINED_VAR}"`,
404+
envVars: map[string]string{
405+
"DEFINED_VAR": "defined-value",
406+
},
407+
expected: map[string]ServerExecutionContext{
408+
"test-server": {
409+
Name: "test-server",
410+
Args: []string{"--missing", "", "--empty="},
411+
Env: map[string]string{
412+
"MISSING_VALUE": "",
413+
"PRESENT_VALUE": "defined-value",
414+
},
415+
},
416+
},
417+
},
418+
{
419+
name: "multiple servers with different expansions",
420+
content: `[servers.server-a]
421+
args = ["--port", "${SERVER_A_PORT}"]
422+
[servers.server-a.env]
423+
SERVICE_NAME = "${SERVER_A_NAME}"
424+
425+
[servers.server-b]
426+
args = ["--port", "${SERVER_B_PORT}"]
427+
[servers.server-b.env]
428+
SERVICE_NAME = "${SERVER_B_NAME}"`,
429+
envVars: map[string]string{
430+
"SERVER_A_PORT": "3000",
431+
"SERVER_A_NAME": "service-alpha",
432+
"SERVER_B_PORT": "4000",
433+
"SERVER_B_NAME": "service-beta",
434+
},
435+
expected: map[string]ServerExecutionContext{
436+
"server-a": {
437+
Name: "server-a",
438+
Args: []string{"--port", "3000"},
439+
Env: map[string]string{
440+
"SERVICE_NAME": "service-alpha",
441+
},
442+
},
443+
"server-b": {
444+
Name: "server-b",
445+
Args: []string{"--port", "4000"},
446+
Env: map[string]string{
447+
"SERVICE_NAME": "service-beta",
448+
},
449+
},
450+
},
451+
},
452+
{
453+
name: "complex variable references",
454+
content: `[servers.complex-server]
455+
args = ["--url", "${PROTO}://${HOST}:${PORT}${PATH}", "--config", "${HOME}/.config/app.json"]
456+
[servers.complex-server.env]
457+
FULL_URL = "${PROTO}://${HOST}:${PORT}${PATH}"
458+
HOME_CONFIG = "${HOME}/.config"`,
459+
envVars: map[string]string{
460+
"PROTO": "https",
461+
"HOST": "api.example.com",
462+
"PORT": "443",
463+
"PATH": "/v1/api",
464+
"HOME": "/home/user",
465+
},
466+
expected: map[string]ServerExecutionContext{
467+
"complex-server": {
468+
Name: "complex-server",
469+
Args: []string{
470+
"--url",
471+
"https://api.example.com:443/v1/api",
472+
"--config",
473+
"/home/user/.config/app.json",
474+
},
475+
Env: map[string]string{
476+
"FULL_URL": "https://api.example.com:443/v1/api",
477+
"HOME_CONFIG": "/home/user/.config",
478+
},
479+
},
480+
},
481+
},
482+
}
483+
484+
for _, tc := range testCases {
485+
t.Run(tc.name, func(t *testing.T) {
486+
// Note: Cannot use t.Parallel() with t.Setenv
487+
488+
// Set up environment variables.
489+
for k, v := range tc.envVars {
490+
t.Setenv(k, v)
491+
}
492+
493+
// Create a temporary config file.
494+
tmpDir := t.TempDir()
495+
configPath := filepath.Join(tmpDir, "test-config.toml")
496+
err := os.WriteFile(configPath, []byte(tc.content), 0o644)
497+
require.NoError(t, err)
498+
499+
// Load the config.
500+
cfg, err := loadExecutionContextConfig(configPath)
501+
require.NoError(t, err)
502+
require.NotNil(t, cfg)
503+
504+
// Verify the expanded results.
505+
require.Equal(t, len(tc.expected), len(cfg.Servers), "Should have expected number of servers")
506+
507+
for expectedName, expectedServer := range tc.expected {
508+
actualServer, exists := cfg.Servers[expectedName]
509+
require.True(t, exists, "Server %s should exist", expectedName)
510+
511+
// Check server name.
512+
require.Equal(t, expectedServer.Name, actualServer.Name, "Server name should match")
513+
514+
// Check args.
515+
require.Equal(
516+
t,
517+
expectedServer.Args,
518+
actualServer.Args,
519+
"Args should be expanded correctly for server %s",
520+
expectedName,
521+
)
522+
523+
// Check env vars.
524+
require.Equal(
525+
t,
526+
expectedServer.Env,
527+
actualServer.Env,
528+
"Env vars should be expanded correctly for server %s",
529+
expectedName,
530+
)
531+
}
532+
})
533+
}
534+
}
535+
536+
// TestLoadExecutionContextConfig_NoExpansionWhenNoVars tests that files without variables work normally.
537+
func TestLoadExecutionContextConfig_NoExpansionWhenNoVars(t *testing.T) {
538+
t.Parallel()
539+
540+
content := `[servers.simple-server]
541+
args = ["--port", "3000", "--debug"]
542+
[servers.simple-server.env]
543+
NODE_ENV = "development"
544+
DEBUG = "true"`
545+
546+
tmpDir := t.TempDir()
547+
configPath := filepath.Join(tmpDir, "simple-config.toml")
548+
err := os.WriteFile(configPath, []byte(content), 0o644)
549+
require.NoError(t, err)
550+
551+
cfg, err := loadExecutionContextConfig(configPath)
552+
require.NoError(t, err)
553+
require.NotNil(t, cfg)
554+
555+
expected := map[string]ServerExecutionContext{
556+
"simple-server": {
557+
Name: "simple-server",
558+
Args: []string{"--port", "3000", "--debug"},
559+
Env: map[string]string{
560+
"NODE_ENV": "development",
561+
"DEBUG": "true",
562+
},
563+
},
564+
}
565+
566+
require.Equal(t, expected, cfg.Servers)
567+
}
568+
569+
// TestLoadExecutionContextConfig_UndefinedVariables tests that undefined environment variables expand to empty strings.
570+
func TestLoadExecutionContextConfig_UndefinedVariables(t *testing.T) {
571+
t.Parallel()
572+
573+
content := `[servers.test-server]
574+
args = ["--token", "${UNDEFINED_TOKEN}", "--config=${UNDEFINED_CONFIG_PATH}"]
575+
[servers.test-server.env]
576+
API_KEY = "${UNDEFINED_API_KEY}"
577+
DATABASE_URL = "${UNDEFINED_DB_URL}"`
578+
579+
tmpDir := t.TempDir()
580+
configPath := filepath.Join(tmpDir, "undefined-vars-config.toml")
581+
err := os.WriteFile(configPath, []byte(content), 0o644)
582+
require.NoError(t, err)
583+
584+
// Deliberately NOT setting any environment variables
585+
cfg, err := loadExecutionContextConfig(configPath)
586+
require.NoError(t, err)
587+
require.NotNil(t, cfg)
588+
589+
server, exists := cfg.Servers["test-server"]
590+
require.True(t, exists, "test-server should exist")
591+
592+
// All undefined variables should expand to empty strings
593+
require.Equal(t, []string{"--token", "", "--config="}, server.Args, "Undefined vars in args should expand to empty strings")
594+
require.Equal(t, map[string]string{
595+
"API_KEY": "",
596+
"DATABASE_URL": "",
597+
}, server.Env, "Undefined vars in env should expand to empty strings")
598+
}
599+
600+
// TestLoadExecutionContextConfig_EmptyFile tests loading an empty config file.
601+
func TestLoadExecutionContextConfig_EmptyFile(t *testing.T) {
602+
t.Parallel()
603+
604+
tmpDir := t.TempDir()
605+
configPath := filepath.Join(tmpDir, "empty-config.toml")
606+
err := os.WriteFile(configPath, []byte(""), 0o644)
607+
require.NoError(t, err)
608+
609+
cfg, err := loadExecutionContextConfig(configPath)
610+
require.NoError(t, err)
611+
require.NotNil(t, cfg)
612+
require.Empty(t, cfg.Servers, "Empty file should result in no servers")
613+
}

‎internal/daemon/daemon.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ func (d *Daemon) startMCPServer(ctx context.Context, server runtime.Server) erro
178178
args = append(args, "-y")
179179
}
180180
args = append(args, packageNameAndVersion)
181-
args = append(args, server.ResolvedArgs()...)
181+
args = append(args, server.Args...)
182182

183183
logger.Debug("attempting to start server", "binary", runtimeBinary)
184184

0 commit comments

Comments
 (0)