@@ -91,17 +91,40 @@ func TestCreateTransport_URLResolution(t *testing.T) {
91 require.Equal(t, "https://mcp.example.com/events", sse.Endpoint)
91 require.Equal(t, "https://mcp.example.com/events", sse.Endpoint)
94− t.Run("http unset var surfaces error, no transport created", func(t *testing.T) {
94+ t.Run("http failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
95 t.Parallel()
95 t.Parallel()
96+ // Under lenient nounset, unset $VAR expands to "" silently,
97+ // so the only way a URL resolution *errors* is a failing
98+ // $(cmd). Mirror the SSE subtest so both transports share
99+ // coverage for the url-resolve-failure path.
96 m := config.MCPConfig{
100 m := config.MCPConfig{
97 Type: config.MCPHttp,
101 Type: config.MCPHttp,
98− URL: "https://$MCP_MISSING_HOST/api",
102+ URL: "https://$(false)/api",
100− tr, err := createTransport(t.Context(), m, shell)
104+ tr, err := createTransport(t.Context(), m, shellResolverWithPath(t, nil))
101 require.Error(t, err)
105 require.Error(t, err)
102 require.Nil(t, tr)
106 require.Nil(t, tr)
103 require.Contains(t, err.Error(), "url:")
107 require.Contains(t, err.Error(), "url:")
104− require.Contains(t, err.Error(), "$MCP_MISSING_HOST")
108+ require.Contains(t, err.Error(), "$(false)")
111+ t.Run("http unset var expands empty", func(t *testing.T) {
113+ // Pinning test for the new lenient-nounset default: an
114+ // unset bare $VAR in the URL is *not* an error. It
115+ // expands to "" and, here, leaves a syntactically weird
116+ // but non-empty URL that the existing non-empty guard
117+ // still lets through. Guards against a future regression
118+ // that flips strict-by-default back on.
119+ m := config.MCPConfig{
120+ Type: config.MCPHttp,
121+ URL: "https://$MCP_MISSING_HOST/api",
123+ tr, err := createTransport(t.Context(), m, shell)
124+ require.NoError(t, err)
125+ sct, ok := tr.(*mcp.StreamableClientTransport)
127+ require.Equal(t, "https:///api", sct.Endpoint)
107 t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
130 t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
@@ -201,16 +224,18 @@ func TestCreateTransport_StdioResolution(t *testing.T) {
201 require.Contains(t, err.Error(), "env TOKEN")
224 require.Contains(t, err.Error(), "env TOKEN")
204− t.Run("unset env var is a hard error, not silent empty", func(t *testing.T) {
227+ t.Run("failing env command is a hard error", func(t *testing.T) {
205 t.Parallel()
228 t.Parallel()
206− // The regression at the heart of PLAN.md: unset vars used to
229+ // Under lenient nounset a bare $UNSET expands to ""
230+ // silently — see the pinning subtest below. The remaining
231+ // failure mode for env resolution is a $(cmd) that exits
232+ // non-zero, which must still error out and prevent exec so
207− // silently substitute "" and hand an empty credential to the
233+ // we never hand a broken credential to the child process.
208− // child process. Now they must error out before exec.
209 r := shellResolverWithPath(t, nil)
234 r := shellResolverWithPath(t, nil)
210 m := config.MCPConfig{
235 m := config.MCPConfig{
211 Type: config.MCPStdio,
236 Type: config.MCPStdio,
212 Command: "forgejo-mcp",
237 Command: "forgejo-mcp",
213− Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN"},
238+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$(exit 5)"},
215 tr, err := createTransport(t.Context(), m, r)
240 tr, err := createTransport(t.Context(), m, r)
216 require.Error(t, err)
241 require.Error(t, err)
@@ -218,6 +243,28 @@ func TestCreateTransport_StdioResolution(t *testing.T) {
218 require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN")
243 require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN")
246+ t.Run("unset env var expands empty", func(t *testing.T) {
248+ // Pinning test for the lenient-nounset default: a bare
249+ // $UNSET in an env value expands to "" without error, and
250+ // the empty entry is kept on the resulting exec.Cmd (env
251+ // entries, unlike headers, are not dropped — see design
252+ // decision #18). Guards against a regression that flips
253+ // strict-by-default back on and silently breaks users
254+ // with configs like FORGEJO_ACCESS_TOKEN=$FORGEJO_TOKEN.
255+ r := shellResolverWithPath(t, nil)
256+ m := config.MCPConfig{
257+ Type: config.MCPStdio,
258+ Command: "forgejo-mcp",
259+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGEJO_TOKEN_UNSET"},
261+ tr, err := createTransport(t.Context(), m, r)
262+ require.NoError(t, err)
263+ ct, ok := tr.(*mcp.CommandTransport)
265+ require.Contains(t, ct.Command.Env, "FORGEJO_ACCESS_TOKEN=")
221 t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) {
268 t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) {
222 t.Parallel()
269 t.Parallel()
223 r := shellResolverWithPath(t, nil)
270 r := shellResolverWithPath(t, nil)
@@ -311,17 +358,43 @@ func TestCreateTransport_HeadersResolution(t *testing.T) {
312 t.Run("sse failing header surfaces error, no transport", func(t *testing.T) {
359 t.Run("sse failing header surfaces error, no transport", func(t *testing.T) {
313 t.Parallel()
360 t.Parallel()
361+ // Under lenient nounset a bare $MISSING expands to "",
362+ // which ResolvedHeaders drops — no error. The failing
363+ // $(cmd) path is the remaining way this can fail loudly;
364+ // cover it on the SSE branch to mirror the HTTP subtest.
314 r := shellResolverWithPath(t, nil)
365 r := shellResolverWithPath(t, nil)
315 m := config.MCPConfig{
366 m := config.MCPConfig{
316 Type: config.MCPSSE,
367 Type: config.MCPSSE,
317 URL: "https://mcp.example.com/events",
368 URL: "https://mcp.example.com/events",
318− Headers: map[string]string{"Authorization": "Bearer $MISSING_TOKEN"},
369+ Headers: map[string]string{"Authorization": "$(false)"},
320 tr, err := createTransport(t.Context(), m, r)
371 tr, err := createTransport(t.Context(), m, r)
321 require.Error(t, err)
372 require.Error(t, err)
322 require.Nil(t, tr)
373 require.Nil(t, tr)
323 require.Contains(t, err.Error(), "header Authorization")
374 require.Contains(t, err.Error(), "header Authorization")
324− require.Contains(t, err.Error(), "$MISSING_TOKEN")
375+ })
377+ t.Run("sse unset var header drops silently", func(t *testing.T) {
379+ // Pinning test for design decision #18 + lenient nounset:
380+ // a header whose value resolves to "" (here because the
381+ // bare $VAR is unset) is omitted from the round tripper
382+ // rather than sent as "X-Header:". Guards against a
383+ // regression that either re-introduces strict-by-default
384+ // or stops dropping empty headers.
385+ r := shellResolverWithPath(t, nil)
386+ m := config.MCPConfig{
387+ Type: config.MCPSSE,
388+ URL: "https://mcp.example.com/events",
389+ Headers: map[string]string{"Authorization": "$MISSING_TOKEN"},
391+ tr, err := createTransport(t.Context(), m, r)
392+ require.NoError(t, err)
393+ sse, ok := tr.(*mcp.SSEClientTransport)
395+ rt, ok := sse.HTTPClient.Transport.(*headerRoundTripper)
397+ require.NotContains(t, rt.headers, "Authorization")
@@ -356,24 +429,44 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
356 wantErrContains: "env FORGEJO_ACCESS_TOKEN",
429 wantErrContains: "env FORGEJO_ACCESS_TOKEN",
432+ // Args that reference an unset bare $VAR no longer
433+ // error out under lenient nounset; the only remaining
434+ // failure mode for arg resolution is a failing $(cmd).
359 name: "stdio args failure",
435 name: "stdio args failure",
360 mcpName: "test-stdio-args-fail",
436 mcpName: "test-stdio-args-fail",
361 cfg: config.MCPConfig{
437 cfg: config.MCPConfig{
362 Type: config.MCPStdio,
438 Type: config.MCPStdio,
363 Command: "echo",
439 Command: "echo",
364− Args: []string{"--token", "$MCP_UNSET_TOKEN"},
440+ Args: []string{"--token", "$(false)"},
366 wantErrContains: "arg 1",
442 wantErrContains: "arg 1",
445+ // Likewise for URL: bare $UNSET expands to ""
446+ // silently, so we need a failing $(cmd) to exercise
447+ // the "url:" wrap from ResolvedURL.
369 name: "http url failure",
448 name: "http url failure",
370 mcpName: "test-http-url-fail",
449 mcpName: "test-http-url-fail",
371 cfg: config.MCPConfig{
450 cfg: config.MCPConfig{
372 Type: config.MCPHttp,
451 Type: config.MCPHttp,
373− URL: "https://$MCP_MISSING_HOST/api",
452+ URL: "https://$(false)/api",
375 wantErrContains: "url:",
454 wantErrContains: "url:",
457+ // A URL whose shell expansion yields the empty
458+ // string (here via ${VAR:-}) is not a ResolvedURL
459+ // error, but the non-empty guard in createTransport
460+ // must still reject it so the state card renders an
461+ // error instead of spawning a transport against "".
462+ name: "http empty-resolved url",
463+ mcpName: "test-http-url-empty",
464+ cfg: config.MCPConfig{
465+ Type: config.MCPHttp,
466+ URL: "${MCP_URL_EMPTY:-}",
468+ wantErrContains: "non-empty 'url'",
378 name: "http header failure",
471 name: "http header failure",
379 mcpName: "test-http-header-fail",
472 mcpName: "test-http-header-fail",
@@ -394,12 +487,17 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
394 wantErrContains: "url:",
487 wantErrContains: "url:",
490+ // Bare $MISSING in a header resolves to "" silently
491+ // and is then dropped (design decision #18). The
492+ // "header Authorization" wrap only surfaces on a
493+ // $(cmd) failure; that is what this subtest now
494+ // pins for the SSE path.
397 name: "sse header failure",
495 name: "sse header failure",
398 mcpName: "test-sse-header-fail",
496 mcpName: "test-sse-header-fail",
399 cfg: config.MCPConfig{
497 cfg: config.MCPConfig{
400 Type: config.MCPSSE,
498 Type: config.MCPSSE,
401 URL: "https://mcp.example.com/events",
499 URL: "https://mcp.example.com/events",
402− Headers: map[string]string{"Authorization": "Bearer $MISSING_SSE_TOKEN"},
500+ Headers: map[string]string{"Authorization": "$(false)"},
404 wantErrContains: "header Authorization",
502 wantErrContains: "header Authorization",