Skip to content

Commit 4444354

Browse files
authored
Fix config export for positional arguments (#188)
* Linting * Fix export of positional arguments in config export Refactored exportRuntimeArgs to properly handle positional arguments that were previously being skipped. Split logic into clean helper functions following functional programming principles. - Add partitionArgs() to separate positional and flag arguments - Add exportPositionalArgs() to handle ARG_1, ARG_2, etc. - Add exportFlagArgs() for flag argument processing - Add comprehensive test coverage for all scenarios
1 parent 1bd6fd2 commit 4444354

File tree

3 files changed

+193
-35
lines changed

3 files changed

+193
-35
lines changed

internal/daemon/daemon.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,17 @@ func (d *Daemon) startMCPServer(ctx context.Context, server runtime.Server) erro
269269
return fmt.Errorf("error initializing MCP client: '%s': %w", server.Name(), err)
270270
}
271271

272-
logger.Info("Initialized", "package", pkg, "version", ver, "server-name", initResult.ServerInfo.Name, "server-version", initResult.ServerInfo.Version)
272+
logger.Info(
273+
"Initialized",
274+
"package",
275+
pkg,
276+
"version",
277+
ver,
278+
"server-name",
279+
initResult.ServerInfo.Name,
280+
"server-version",
281+
initResult.ServerInfo.Version,
282+
)
273283

274284
// Store and track the client.
275285
d.clientManager.Add(server.Name(), stdioClient, server.Tools)

internal/runtime/server.go

Lines changed: 94 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -300,57 +300,120 @@ func (s *Server) exportEnvVars(appName string) map[string]string {
300300
return envs
301301
}
302302

303-
func (s *Server) exportRuntimeArgs(
303+
// partitionArgs separates arguments into positional (non-flag) and flag arguments.
304+
// Positional args come before any flags. Once a flag is encountered, all remaining
305+
// args are treated as flags or flag values.
306+
func partitionArgs(args []string) (positional []string, flags []string) {
307+
for i, arg := range args {
308+
if strings.HasPrefix(arg, "--") {
309+
// split here
310+
positional = args[:i]
311+
flags = args[i:]
312+
return
313+
}
314+
}
315+
// no flags found
316+
positional = args
317+
return
318+
}
319+
320+
// exportPositionalArgs transforms positional arguments into environment variable placeholders.
321+
// Arguments are numbered starting from 1 (ARG_1, ARG_2, etc.).
322+
func (s *Server) exportPositionalArgs(
323+
positional []string,
304324
appName string,
305-
seen map[string]struct{},
306325
recordContractFunc func(k, v string),
307326
) []string {
308-
var args []string
327+
if len(positional) == 0 {
328+
return nil
329+
}
309330

310-
// Filter out args with cross-server references.
311-
filteredArgs := s.filterArgs(s.Args)
331+
result := make([]string, 0, len(positional))
332+
for i := range positional {
333+
// Number positional args starting from 1.
334+
argKey := fmt.Sprintf("ARG_%d", i+1)
335+
envVarName := buildEnvVarName(appName, s.Name(), argKey)
336+
envVarRef := fmt.Sprintf("${%s}", envVarName)
337+
result = append(result, envVarRef)
338+
recordContractFunc(envVarName, envVarRef)
339+
}
340+
return result
341+
}
312342

313-
for i := 0; i < len(filteredArgs); i++ {
314-
rawArg := filteredArgs[i]
343+
// exportFlagArgs processes flag arguments and transforms them into environment variable placeholders.
344+
func (s *Server) exportFlagArgs(
345+
flags []string,
346+
appName string,
347+
seen map[string]struct{},
348+
recordContractFunc func(k, v string),
349+
) []string {
350+
if len(flags) == 0 {
351+
return nil
352+
}
315353

316-
// Sanity check for arg.
317-
if !strings.HasPrefix(rawArg, "--") {
318-
continue // value
319-
}
354+
var result []string
355+
for i := 0; i < len(flags); i++ {
356+
flag := flags[i]
320357

321-
arg := extractArgNameWithPrefix(rawArg)
322-
if _, ok := seen[arg]; ok {
323-
continue // Already handled.
358+
// Skip non-flag arguments (values).
359+
if !strings.HasPrefix(flag, "--") {
360+
continue
324361
}
325362

326-
// --arg=val case
327-
if strings.HasPrefix(rawArg, arg+"=") {
328-
t := transformValueArg(appName, s.Name(), rawArg)
329-
args = append(args, t.FormattedArg)
330-
recordContractFunc(t.EnvVarName, t.EnvVarReference)
331-
seen[arg] = struct{}{}
363+
argName := extractArgNameWithPrefix(flag)
364+
365+
// Skip if already processed.
366+
if _, ok := seen[argName]; ok {
332367
continue
333368
}
334369

335-
// --arg val case
336-
if rawArg == arg && i+1 < len(filteredArgs) && !strings.HasPrefix(filteredArgs[i+1], "--") {
337-
t := transformValueArg(appName, s.Name(), rawArg)
338-
args = append(args, t.FormattedArg)
370+
// Handle --arg=value format.
371+
if strings.Contains(flag, "=") {
372+
t := transformValueArg(appName, s.Name(), flag)
373+
result = append(result, t.FormattedArg)
339374
recordContractFunc(t.EnvVarName, t.EnvVarReference)
340-
seen[arg] = struct{}{}
341-
i++ // Skip the next item since it's an actual value.
375+
seen[argName] = struct{}{}
342376
continue
343377
}
344378

345-
// bool flag
346-
if rawArg == arg {
347-
args = append(args, arg)
348-
seen[arg] = struct{}{}
379+
// Handle --arg value format (check next item).
380+
if i+1 < len(flags) && !strings.HasPrefix(flags[i+1], "--") {
381+
t := transformValueArg(appName, s.Name(), flag)
382+
result = append(result, t.FormattedArg)
383+
recordContractFunc(t.EnvVarName, t.EnvVarReference)
384+
seen[argName] = struct{}{}
385+
i++ // Skip the value.
349386
continue
350387
}
388+
389+
// Handle boolean flag.
390+
result = append(result, flag)
391+
seen[argName] = struct{}{}
351392
}
352393

353-
return args
394+
return result
395+
}
396+
397+
func (s *Server) exportRuntimeArgs(
398+
appName string,
399+
seen map[string]struct{},
400+
recordContractFunc func(k, v string),
401+
) []string {
402+
// Filter out args with cross-server references.
403+
filteredArgs := s.filterArgs(s.Args)
404+
if len(filteredArgs) == 0 {
405+
return nil
406+
}
407+
408+
// Partition into positional and flag arguments.
409+
positional, flags := partitionArgs(filteredArgs)
410+
411+
// Process each type separately and combine results.
412+
var result []string
413+
result = append(result, s.exportPositionalArgs(positional, appName, recordContractFunc)...)
414+
result = append(result, s.exportFlagArgs(flags, appName, seen, recordContractFunc)...)
415+
416+
return result
354417
}
355418

356419
func (s *Servers) Export(path string) (map[string]string, error) {

internal/runtime/server_test.go

Lines changed: 88 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,72 @@ func TestValidate(t *testing.T) {
811811
}
812812
}
813813

814+
func TestPartitionArgs(t *testing.T) {
815+
t.Parallel()
816+
817+
tests := []struct {
818+
name string
819+
args []string
820+
expectedPositional []string
821+
expectedFlags []string
822+
}{
823+
{
824+
name: "empty args",
825+
args: []string{},
826+
expectedPositional: []string{},
827+
},
828+
{
829+
name: "only positional args",
830+
args: []string{"file1", "file2", "file3"},
831+
expectedPositional: []string{"file1", "file2", "file3"},
832+
},
833+
{
834+
name: "only flags",
835+
args: []string{"--flag1", "--flag2=value", "--flag3", "value3"},
836+
expectedPositional: []string{},
837+
expectedFlags: []string{"--flag1", "--flag2=value", "--flag3", "value3"},
838+
},
839+
{
840+
name: "positional then flags",
841+
args: []string{"pos1", "pos2", "--flag1", "--flag2=value"},
842+
expectedPositional: []string{"pos1", "pos2"},
843+
expectedFlags: []string{"--flag1", "--flag2=value"},
844+
},
845+
{
846+
name: "positional then flag with value",
847+
args: []string{"pos1", "--flag", "value", "--another"},
848+
expectedPositional: []string{"pos1"},
849+
expectedFlags: []string{"--flag", "value", "--another"},
850+
},
851+
{
852+
name: "no positional after first flag",
853+
args: []string{"pos1", "--flag", "value", "should-be-flag-value"},
854+
expectedPositional: []string{"pos1"},
855+
expectedFlags: []string{"--flag", "value", "should-be-flag-value"},
856+
},
857+
}
858+
859+
for _, tc := range tests {
860+
t.Run(tc.name, func(t *testing.T) {
861+
t.Parallel()
862+
863+
positional, flags := partitionArgs(tc.args)
864+
865+
if tc.expectedPositional == nil {
866+
require.Nil(t, positional)
867+
} else {
868+
require.Equal(t, tc.expectedPositional, positional)
869+
}
870+
871+
if tc.expectedFlags == nil {
872+
require.Nil(t, flags)
873+
} else {
874+
require.Equal(t, tc.expectedFlags, flags)
875+
}
876+
})
877+
}
878+
}
879+
814880
func TestServer_exportRuntimeArgs(t *testing.T) {
815881
t.Parallel()
816882

@@ -880,15 +946,34 @@ func TestServer_exportRuntimeArgs(t *testing.T) {
880946
},
881947
},
882948
{
883-
name: "skip non-flag arguments",
884-
args: []string{"--host", "localhost", "not-a-flag", "--debug"},
949+
name: "positional arguments before flags",
950+
args: []string{"/path/to/file", "second-positional", "--host", "localhost", "--debug"},
885951
seen: map[string]struct{}{},
886952
expectedArgs: []string{
953+
"${MCPD__TEST_SERVER__ARG_1}",
954+
"${MCPD__TEST_SERVER__ARG_2}",
887955
"--host=${MCPD__TEST_SERVER__HOST}",
888956
"--debug",
889957
},
890958
expectedCalls: map[string]string{
891-
"MCPD__TEST_SERVER__HOST": "${MCPD__TEST_SERVER__HOST}",
959+
"MCPD__TEST_SERVER__ARG_1": "${MCPD__TEST_SERVER__ARG_1}",
960+
"MCPD__TEST_SERVER__ARG_2": "${MCPD__TEST_SERVER__ARG_2}",
961+
"MCPD__TEST_SERVER__HOST": "${MCPD__TEST_SERVER__HOST}",
962+
},
963+
},
964+
{
965+
name: "only positional arguments",
966+
args: []string{"/path/to/file", "second-arg", "third-arg"},
967+
seen: map[string]struct{}{},
968+
expectedArgs: []string{
969+
"${MCPD__TEST_SERVER__ARG_1}",
970+
"${MCPD__TEST_SERVER__ARG_2}",
971+
"${MCPD__TEST_SERVER__ARG_3}",
972+
},
973+
expectedCalls: map[string]string{
974+
"MCPD__TEST_SERVER__ARG_1": "${MCPD__TEST_SERVER__ARG_1}",
975+
"MCPD__TEST_SERVER__ARG_2": "${MCPD__TEST_SERVER__ARG_2}",
976+
"MCPD__TEST_SERVER__ARG_3": "${MCPD__TEST_SERVER__ARG_3}",
892977
},
893978
},
894979
{

0 commit comments

Comments
 (0)