@@ -91,17 +91,40 @@ func TestCreateTransport_URLResolution(t *testing.T) {
require.Equal(t, "https://mcp.example.com/events", sse.Endpoint)
})
- t.Run("http unset var surfaces error, no transport created", func(t *testing.T) {
+ t.Run("http failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
t.Parallel()
+ // Under lenient nounset, unset $VAR expands to "" silently,
+ // so the only way a URL resolution *errors* is a failing
+ // $(cmd). Mirror the SSE subtest so both transports share
+ // coverage for the url-resolve-failure path.
m := config.MCPConfig{
Type: config.MCPHttp,
- URL: "https://$MCP_MISSING_HOST/api",
+ URL: "https://$(false)/api",
}
- tr, err := createTransport(t.Context(), m, shell)
+ tr, err := createTransport(t.Context(), m, shellResolverWithPath(t, nil))
require.Error(t, err)
require.Nil(t, tr)
require.Contains(t, err.Error(), "url:")
- require.Contains(t, err.Error(), "$MCP_MISSING_HOST")
+ require.Contains(t, err.Error(), "$(false)")
+ })
+
+ t.Run("http unset var expands empty", func(t *testing.T) {
+ t.Parallel()
+ // Pinning test for the new lenient-nounset default: an
+ // unset bare $VAR in the URL is *not* an error. It
+ // expands to "" and, here, leaves a syntactically weird
+ // but non-empty URL that the existing non-empty guard
+ // still lets through. Guards against a future regression
+ // that flips strict-by-default back on.
+ m := config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "https://$MCP_MISSING_HOST/api",
+ }
+ tr, err := createTransport(t.Context(), m, shell)
+ require.NoError(t, err)
+ sct, ok := tr.(*mcp.StreamableClientTransport)
+ require.True(t, ok)
+ require.Equal(t, "https:///api", sct.Endpoint)
})
t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
@@ -201,16 +224,18 @@ func TestCreateTransport_StdioResolution(t *testing.T) {
require.Contains(t, err.Error(), "env TOKEN")
})
- t.Run("unset env var is a hard error, not silent empty", func(t *testing.T) {
+ t.Run("failing env command is a hard error", func(t *testing.T) {
t.Parallel()
- // The regression at the heart of PLAN.md: unset vars used to
- // silently substitute "" and hand an empty credential to the
- // child process. Now they must error out before exec.
+ // Under lenient nounset a bare $UNSET expands to ""
+ // silently — see the pinning subtest below. The remaining
+ // failure mode for env resolution is a $(cmd) that exits
+ // non-zero, which must still error out and prevent exec so
+ // we never hand a broken credential to the child process.
r := shellResolverWithPath(t, nil)
m := config.MCPConfig{
Type: config.MCPStdio,
Command: "forgejo-mcp",
- Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN"},
+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$(exit 5)"},
}
tr, err := createTransport(t.Context(), m, r)
require.Error(t, err)
@@ -218,6 +243,28 @@ func TestCreateTransport_StdioResolution(t *testing.T) {
require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN")
})
+ t.Run("unset env var expands empty", func(t *testing.T) {
+ t.Parallel()
+ // Pinning test for the lenient-nounset default: a bare
+ // $UNSET in an env value expands to "" without error, and
+ // the empty entry is kept on the resulting exec.Cmd (env
+ // entries, unlike headers, are not dropped — see design
+ // decision #18). Guards against a regression that flips
+ // strict-by-default back on and silently breaks users
+ // with configs like FORGEJO_ACCESS_TOKEN=$FORGEJO_TOKEN.
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "forgejo-mcp",
+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGEJO_TOKEN_UNSET"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.NoError(t, err)
+ ct, ok := tr.(*mcp.CommandTransport)
+ require.True(t, ok)
+ require.Contains(t, ct.Command.Env, "FORGEJO_ACCESS_TOKEN=")
+ })
+
t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) {
t.Parallel()
r := shellResolverWithPath(t, nil)
@@ -311,17 +358,43 @@ func TestCreateTransport_HeadersResolution(t *testing.T) {
t.Run("sse failing header surfaces error, no transport", func(t *testing.T) {
t.Parallel()
+ // Under lenient nounset a bare $MISSING expands to "",
+ // which ResolvedHeaders drops — no error. The failing
+ // $(cmd) path is the remaining way this can fail loudly;
+ // cover it on the SSE branch to mirror the HTTP subtest.
r := shellResolverWithPath(t, nil)
m := config.MCPConfig{
Type: config.MCPSSE,
URL: "https://mcp.example.com/events",
- Headers: map[string]string{"Authorization": "Bearer $MISSING_TOKEN"},
+ Headers: map[string]string{"Authorization": "$(false)"},
}
tr, err := createTransport(t.Context(), m, r)
require.Error(t, err)
require.Nil(t, tr)
require.Contains(t, err.Error(), "header Authorization")
- require.Contains(t, err.Error(), "$MISSING_TOKEN")
+ })
+
+ t.Run("sse unset var header drops silently", func(t *testing.T) {
+ t.Parallel()
+ // Pinning test for design decision #18 + lenient nounset:
+ // a header whose value resolves to "" (here because the
+ // bare $VAR is unset) is omitted from the round tripper
+ // rather than sent as "X-Header:". Guards against a
+ // regression that either re-introduces strict-by-default
+ // or stops dropping empty headers.
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPSSE,
+ URL: "https://mcp.example.com/events",
+ Headers: map[string]string{"Authorization": "$MISSING_TOKEN"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.NoError(t, err)
+ sse, ok := tr.(*mcp.SSEClientTransport)
+ require.True(t, ok)
+ rt, ok := sse.HTTPClient.Transport.(*headerRoundTripper)
+ require.True(t, ok)
+ require.NotContains(t, rt.headers, "Authorization")
})
}
@@ -356,24 +429,44 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
wantErrContains: "env FORGEJO_ACCESS_TOKEN",
},
{
+ // Args that reference an unset bare $VAR no longer
+ // error out under lenient nounset; the only remaining
+ // failure mode for arg resolution is a failing $(cmd).
name: "stdio args failure",
mcpName: "test-stdio-args-fail",
cfg: config.MCPConfig{
Type: config.MCPStdio,
Command: "echo",
- Args: []string{"--token", "$MCP_UNSET_TOKEN"},
+ Args: []string{"--token", "$(false)"},
},
wantErrContains: "arg 1",
},
{
+ // Likewise for URL: bare $UNSET expands to ""
+ // silently, so we need a failing $(cmd) to exercise
+ // the "url:" wrap from ResolvedURL.
name: "http url failure",
mcpName: "test-http-url-fail",
cfg: config.MCPConfig{
Type: config.MCPHttp,
- URL: "https://$MCP_MISSING_HOST/api",
+ URL: "https://$(false)/api",
},
wantErrContains: "url:",
},
+ {
+ // A URL whose shell expansion yields the empty
+ // string (here via ${VAR:-}) is not a ResolvedURL
+ // error, but the non-empty guard in createTransport
+ // must still reject it so the state card renders an
+ // error instead of spawning a transport against "".
+ name: "http empty-resolved url",
+ mcpName: "test-http-url-empty",
+ cfg: config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "${MCP_URL_EMPTY:-}",
+ },
+ wantErrContains: "non-empty 'url'",
+ },
{
name: "http header failure",
mcpName: "test-http-header-fail",
@@ -394,12 +487,17 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
wantErrContains: "url:",
},
{
+ // Bare $MISSING in a header resolves to "" silently
+ // and is then dropped (design decision #18). The
+ // "header Authorization" wrap only surfaces on a
+ // $(cmd) failure; that is what this subtest now
+ // pins for the SSE path.
name: "sse header failure",
mcpName: "test-sse-header-fail",
cfg: config.MCPConfig{
Type: config.MCPSSE,
URL: "https://mcp.example.com/events",
- Headers: map[string]string{"Authorization": "Bearer $MISSING_SSE_TOKEN"},
+ Headers: map[string]string{"Authorization": "$(false)"},
},
wantErrContains: "header Authorization",
},