Skip to content

Commit fb2ec03

Browse files
peteski22peteski22
authored
Fix positional arguments issue with config (starting servers, exporting config) (#138)
* Support positional arguments on startup for validation * Support positional arguments correctly in config export * Track required positional arguments in .mcpd.toml * Update terminal text output on successful adding of an MCP server that has positional arguments * Update MCPM registry data used in tests
1 parent db53ac3 commit fb2ec03

File tree

13 files changed

+2550
-816
lines changed

13 files changed

+2550
-816
lines changed

‎cmd/add.go‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -270,17 +270,19 @@ func parseServerEntry(pkg packages.Server, opts serverEntryOptions) (config.Serv
270270
}
271271

272272
runtimePackageVersion := fmt.Sprintf("%s::%s@%s", selectedRuntime, installation.Package, version)
273-
envs := pkg.Arguments.FilterBy(packages.Required, packages.EnvVar)
274-
args := pkg.Arguments.FilterBy(packages.Required, packages.ValueAcceptingArgument)
275-
boolArgs := pkg.Arguments.FilterBy(packages.Required, packages.BoolArgument)
273+
envs := pkg.Arguments.FilterBy(packages.Required, packages.EnvVar).Names()
274+
positionalArgs := pkg.Arguments.FilterBy(packages.Required, packages.PositionalArgument).Ordered().Names()
275+
valueArgs := pkg.Arguments.FilterBy(packages.Required, packages.ValueArgument).Ordered().Names()
276+
boolArgs := pkg.Arguments.FilterBy(packages.Required, packages.BoolArgument).Names()
276277

277278
return config.ServerEntry{
278-
Name: pkg.ID,
279-
Package: runtimePackageVersion,
280-
Tools: requestedTools,
281-
RequiredValueArgs: args.Names(),
282-
RequiredBoolArgs: boolArgs.Names(),
283-
RequiredEnvVars: envs.Names(),
279+
Name: pkg.ID,
280+
Package: runtimePackageVersion,
281+
Tools: requestedTools,
282+
RequiredPositionalArgs: positionalArgs,
283+
RequiredValueArgs: valueArgs,
284+
RequiredBoolArgs: boolArgs,
285+
RequiredEnvVars: envs,
284286
}, nil
285287
}
286288

‎cmd/add_test.go‎

Lines changed: 93 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -197,11 +197,12 @@ func TestAddCmd_ServerWithArguments(t *testing.T) {
197197
t.Parallel()
198198

199199
tests := []struct {
200-
name string
201-
pkg packages.Server
202-
expectedRequiredEnvs []string
203-
expectedRequiredValues []string
204-
expectedRequiredBools []string
200+
name string
201+
pkg packages.Server
202+
expectedRequiredEnvs []string
203+
expectedRequiredPositionals []string
204+
expectedRequiredValues []string
205+
expectedRequiredBools []string
205206
}{
206207
{
207208
name: "server with all argument types",
@@ -307,7 +308,66 @@ func TestAddCmd_ServerWithArguments(t *testing.T) {
307308
"--optional-value": {VariableType: packages.VariableTypeArg, Required: false},
308309
},
309310
},
310-
// All empty since none are required
311+
},
312+
{
313+
name: "server all kinds of required args and envs",
314+
pkg: packages.Server{
315+
ID: "simple-server",
316+
Name: "Simple Server",
317+
Tools: []packages.Tool{
318+
{
319+
Name: "hello",
320+
},
321+
},
322+
Installations: map[runtime.Runtime]packages.Installation{
323+
runtime.UVX: {
324+
Runtime: "uvx",
325+
Package: "mcp-server-simple",
326+
Version: "1.0.0",
327+
Recommended: true,
328+
},
329+
},
330+
Arguments: packages.Arguments{
331+
"REQUIRED_ENV": {
332+
VariableType: packages.VariableTypeEnv,
333+
Required: true,
334+
},
335+
"OPTIONAL_ENV": {
336+
VariableType: packages.VariableTypeEnv,
337+
Required: false,
338+
},
339+
"RequiredPositional1": {
340+
VariableType: packages.VariableTypeArgPositional,
341+
Position: testIntPtr(t, 1),
342+
Required: true,
343+
},
344+
"OptionalPositional2": {
345+
VariableType: packages.VariableTypeArgPositional,
346+
Position: testIntPtr(t, 2),
347+
Required: false,
348+
},
349+
"--required-value": {
350+
VariableType: packages.VariableTypeArg,
351+
Required: true,
352+
},
353+
"--optional-value": {
354+
VariableType: packages.VariableTypeArg,
355+
Required: false,
356+
},
357+
"--required-flag": {
358+
VariableType: packages.VariableTypeArgBool,
359+
Required: true,
360+
},
361+
"--optional-flag": {
362+
VariableType: packages.VariableTypeArgBool,
363+
Required: false,
364+
},
365+
},
366+
},
367+
expectedRequiredEnvs: []string{"REQUIRED_ENV"},
368+
expectedRequiredPositionals: []string{"RequiredPositional1"},
369+
expectedRequiredValues: []string{"--required-value"},
370+
expectedRequiredBools: []string{"--required-flag"},
311371
},
312372
}
313373

@@ -334,6 +394,7 @@ func TestAddCmd_ServerWithArguments(t *testing.T) {
334394
require.True(t, cfg.addCalled)
335395
assert.Equal(t, tc.pkg.ID, cfg.entry.Name)
336396
assert.ElementsMatch(t, tc.expectedRequiredEnvs, cfg.entry.RequiredEnvVars)
397+
assert.ElementsMatch(t, tc.expectedRequiredPositionals, cfg.entry.RequiredPositionalArgs)
337398
assert.ElementsMatch(t, tc.expectedRequiredValues, cfg.entry.RequiredValueArgs)
338399
assert.ElementsMatch(t, tc.expectedRequiredBools, cfg.entry.RequiredBoolArgs)
339400
})
@@ -421,21 +482,22 @@ func TestParseServerEntry(t *testing.T) {
421482
t.Parallel()
422483

423484
tests := []struct {
424-
name string
425-
installations map[runtime.Runtime]packages.Installation
426-
supportedRuntimes []runtime.Runtime
427-
pkgName string
428-
pkgID string
429-
availableTools []string
430-
requestedTools []string
431-
requestedRuntime runtime.Runtime
432-
arguments packages.Arguments
433-
isErrorExpected bool
434-
expectedErrorMessage string
435-
expectedPackageValue string
436-
expectedRequiredEnvs []string
437-
expectedRequiredValues []string
438-
expectedRequiredBools []string
485+
name string
486+
installations map[runtime.Runtime]packages.Installation
487+
supportedRuntimes []runtime.Runtime
488+
pkgName string
489+
pkgID string
490+
availableTools []string
491+
requestedTools []string
492+
requestedRuntime runtime.Runtime
493+
arguments packages.Arguments
494+
isErrorExpected bool
495+
expectedErrorMessage string
496+
expectedPackageValue string
497+
expectedRequiredEnvs []string
498+
expectedRequiredPositionals []string
499+
expectedRequiredValues []string
500+
expectedRequiredBools []string
439501
}{
440502
{
441503
name: "basic server with no arguments",
@@ -548,8 +610,9 @@ func TestParseServerEntry(t *testing.T) {
548610
"--format": {VariableType: packages.VariableTypeArg, Required: true},
549611
"--encoding": {VariableType: packages.VariableTypeArg, Required: false},
550612
},
551-
expectedPackageValue: "uvx::mcp-server-files@latest",
552-
expectedRequiredValues: []string{"path", "mode", "--format"},
613+
expectedPackageValue: "uvx::mcp-server-files@latest",
614+
expectedRequiredPositionals: []string{"path", "mode"},
615+
expectedRequiredValues: []string{"--format"},
553616
},
554617
{
555618
name: "server with only required bool args",
@@ -676,12 +739,13 @@ func TestParseServerEntry(t *testing.T) {
676739
} else {
677740
require.NoError(t, err)
678741
expected := config.ServerEntry{
679-
Name: tc.pkgID,
680-
Package: tc.expectedPackageValue,
681-
Tools: tc.requestedTools,
682-
RequiredEnvVars: tc.expectedRequiredEnvs,
683-
RequiredValueArgs: tc.expectedRequiredValues,
684-
RequiredBoolArgs: tc.expectedRequiredBools,
742+
Name: tc.pkgID,
743+
Package: tc.expectedPackageValue,
744+
Tools: tc.requestedTools,
745+
RequiredEnvVars: tc.expectedRequiredEnvs,
746+
RequiredPositionalArgs: tc.expectedRequiredPositionals,
747+
RequiredValueArgs: tc.expectedRequiredValues,
748+
RequiredBoolArgs: tc.expectedRequiredBools,
685749
}
686750
require.Equal(t, expected.Name, entry.Name)
687751
require.Equal(t, expected.Package, entry.Package)

‎internal/config/types.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,10 @@ type ServerEntry struct {
5353
// RequiredEnvVars captures any environment variables required to run the server.
5454
RequiredEnvVars []string `json:"requiredEnv,omitempty" toml:"required_env,omitempty" yaml:"required_env,omitempty"`
5555

56+
// RequiredPositionalArgs captures any command line args that must be positional, and which are required to run the server.
57+
// The arguments must be ordered by their position (ascending).
58+
RequiredPositionalArgs []string `json:"requiredPositionalArgs,omitempty" toml:"required_args_positional,omitempty"`
59+
5660
// RequiredValueArgs captures any command line args that need values, which are required to run the server.
5761
RequiredValueArgs []string `json:"requiredArgs,omitempty" toml:"required_args,omitempty" yaml:"required_args,omitempty"`
5862

@@ -97,7 +101,15 @@ func (e *argEntry) String() string {
97101
return e.key
98102
}
99103

100-
// RequiredArguments returns all required CLI arguments, including both value-based and boolean flags.
104+
// RequiredArguments returns all required CLI arguments, including positional, value-based and boolean flags.
105+
// NOTE: The order of these arguments matters, so positional arguments appear first.
101106
func (e *ServerEntry) RequiredArguments() []string {
102-
return append(e.RequiredValueArgs, e.RequiredBoolArgs...)
107+
out := make([]string, 0, len(e.RequiredPositionalArgs)+len(e.RequiredValueArgs)+len(e.RequiredBoolArgs))
108+
109+
// Add positional args first.
110+
out = append(out, e.RequiredPositionalArgs...)
111+
out = append(out, e.RequiredValueArgs...)
112+
out = append(out, e.RequiredBoolArgs...)
113+
114+
return out
103115
}

‎internal/packages/argument.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ type VariableType string
3030

3131
type Arguments map[string]ArgumentMetadata
3232

33+
type OrderedArguments []ArgumentMetadata
34+
3335
// ArgumentMetadata represents metadata about an argument/variable
3436
type ArgumentMetadata struct {
3537
// Name is the reference for the argument.
@@ -63,9 +65,18 @@ func (a Arguments) Names() []string {
6365
return slices.Collect(maps.Keys(a))
6466
}
6567

68+
// Names returns the list of names of a collection of OrderedArguments.
69+
func (a OrderedArguments) Names() []string {
70+
orderedArgNames := make([]string, 0, len(a))
71+
for _, arg := range a {
72+
orderedArgNames = append(orderedArgNames, arg.Name)
73+
}
74+
return orderedArgNames
75+
}
76+
6677
// Ordered returns all arguments with positional arguments first (in position order),
6778
// followed by all other arguments in alphabetical order by name.
68-
func (a Arguments) Ordered() []ArgumentMetadata {
79+
func (a Arguments) Ordered() OrderedArguments {
6980
var positional []ArgumentMetadata
7081
var others []ArgumentMetadata
7182

‎internal/printer/entry_printer.go‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,40 @@ func (p *ServerEntryPrinter) Item(w io.Writer, elem config.ServerEntry) error {
5252
_, _ = fmt.Fprint(w, "\nsee: mcpd config env set --help\n")
5353
}
5454

55-
if len(elem.RequiredValueArgs) > 0 || len(elem.RequiredBoolArgs) > 0 {
55+
if len(elem.RequiredPositionalArgs) > 0 || len(elem.RequiredValueArgs) > 0 || len(elem.RequiredBoolArgs) > 0 {
56+
if len(elem.RequiredPositionalArgs) > 0 {
57+
_, _ = fmt.Fprintf(w, "\n❗ The following positional arguments are required for this server:\n\n")
58+
position := 0
59+
parts := make([]string, 0, len(elem.RequiredPositionalArgs))
60+
for _, posArg := range elem.RequiredPositionalArgs {
61+
position++
62+
parts = append(parts, fmt.Sprintf(" 📍 (%d) %s", position, posArg))
63+
}
64+
_, _ = fmt.Fprintln(w, strings.Join(parts, "\n"))
65+
}
66+
5667
if len(elem.RequiredValueArgs) > 0 {
5768
_, _ = fmt.Fprintf(
5869
w,
59-
"\n! The following command line arguments are required (along with values) for this server:\n\n %s\n",
60-
strings.Join(elem.RequiredValueArgs, "\n "),
70+
"\n❗ The following command line arguments are required (along with values) for this server:\n\n",
6171
)
72+
parts := make([]string, 0, len(elem.RequiredValueArgs))
73+
for _, arg := range elem.RequiredValueArgs {
74+
parts = append(parts, fmt.Sprintf(" 🚩 %s", arg))
75+
}
76+
_, _ = fmt.Fprintln(w, strings.Join(parts, "\n"))
6277
}
6378

6479
if len(elem.RequiredBoolArgs) > 0 {
6580
_, _ = fmt.Fprintf(
6681
w,
67-
"\n! The following command line arguments are required (as boolean flags) for this server:\n\n %s\n",
68-
strings.Join(elem.RequiredBoolArgs, "\n "),
82+
"\n❗ The following command line arguments are required (as boolean flags) for this server:\n\n",
6983
)
84+
parts := make([]string, 0, len(elem.RequiredBoolArgs))
85+
for _, arg := range elem.RequiredBoolArgs {
86+
parts = append(parts, fmt.Sprintf(" ✅ %s", arg))
87+
}
88+
_, _ = fmt.Fprintln(w, strings.Join(parts, "\n"))
7089
}
7190

7291
_, _ = fmt.Fprint(w, "\nsee: mcpd config args set --help\n")

0 commit comments

Comments
 (0)