diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 80551a7749014b6f3262d4b0f2b70c26aabde34d..98c00bc553c6468e1663d43fbbc634c82931bf7e 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -1617,10 +1617,14 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) { ExtraHeaders: map[string]string{ // Failing $(...) — inner command exits 1, no stdout. "X-Broken": "$(false)", - // Unset var — nounset makes this an error too. + // Unset var — under Phase 2 lenient nounset this + // resolves cleanly to "" rather than erroring; + // Step 10 will rewrite this whole test once the + // empty-drop rule replaces the log-and-continue + // loop at load.go:225. "X-Missing": "$PROVIDER_HEADER_NEVER_SET", // Happy path: must still be resolved, proving the - // failure in the other two did not abort the loop. + // failure in X-Broken did not abort the loop. "X-Static": "kept-literal", }, }, @@ -1640,10 +1644,15 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) { pc, ok := cfg.Providers.Get("openai") require.True(t, ok, "openai provider must still be configured") - // Literal template preserved for the two failing headers; the - // happy-path header is resolved through the shell (pass-through - // for a literal value). + // X-Broken still errors on $(false) so its literal template is + // preserved by the log-and-continue loop. X-Missing resolves + // cleanly to "" under lenient nounset: the current divergence + // only kicks in on resolver errors, so a non-erroring empty + // expansion writes through. The happy-path header is resolved + // through the shell (pass-through for a literal value). Step 10 + // will flip this whole surface to the MCP contract plus the + // empty-drop rule from design decision #18. require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"]) - require.Equal(t, "$PROVIDER_HEADER_NEVER_SET", pc.ExtraHeaders["X-Missing"]) + require.Equal(t, "", pc.ExtraHeaders["X-Missing"]) require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"]) } diff --git a/internal/config/mcp_resolved_url_test.go b/internal/config/mcp_resolved_url_test.go index 97c0d9df4725164a0ca05933da5df35239fdb691..a1f262f2224b53ed8e7dd1f1044152b853ba90db 100644 --- a/internal/config/mcp_resolved_url_test.go +++ b/internal/config/mcp_resolved_url_test.go @@ -44,14 +44,31 @@ func TestMCPConfig_ResolvedURL(t *testing.T) { require.Equal(t, "https://mcp.example.com/events", got) }) - t.Run("unset var is an error wrapping the template", func(t *testing.T) { + t.Run("unset var expands to empty under lenient default", func(t *testing.T) { t.Parallel() + // Phase 2 defaults to nounset-off: bare $VAR on an unset + // variable expands to "" rather than erroring. Here the + // host collapses to empty, so the caller sees a malformed + // URL rather than a resolver error; that's the expected + // trade-off for making $OPTIONAL-style patterns work, and + // required-credential callers should use ${VAR:?msg}. m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_MISSING_HOST/api"} + got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil))) + require.NoError(t, err, "unset var must not error under lenient default") + require.Equal(t, "https:///api", got) + }) + + t.Run("colon-question on unset var errors regardless of toggle", func(t *testing.T) { + t.Parallel() + // ${VAR:?msg} is the opt-in strictness mechanism; it must + // hard-error even with NoUnset off so required credentials + // surface at load time instead of shipping empty-host URLs + // to the transport layer. + m := MCPConfig{Type: MCPHttp, URL: "https://${MCP_MISSING_HOST:?set MCP_MISSING_HOST}/api"} _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil))) require.Error(t, err) require.Contains(t, err.Error(), "url:") - require.Contains(t, err.Error(), "$MCP_MISSING_HOST") - require.Contains(t, err.Error(), "unbound") + require.Contains(t, err.Error(), "set MCP_MISSING_HOST") }) t.Run("failing command substitution is an error", func(t *testing.T) { diff --git a/internal/config/resolve_real_test.go b/internal/config/resolve_real_test.go index f33435e9615f44a09c9e2ce1692c3b3aa3ebf276..9c9ddd951d3b6ea76be0b92f099e5fcbaa17bd84 100644 --- a/internal/config/resolve_real_test.go +++ b/internal/config/resolve_real_test.go @@ -159,23 +159,50 @@ func TestResolvedEnv_RealShell_Deterministic(t *testing.T) { require.True(t, slices.IsSorted(got), "env slice must be sorted; got %v", got) } -// TestResolvedEnv_RealShell_NounsetRegression is the single most -// important assertion in this file: an unset variable is an error, not -// an empty expansion. Swapping the hand-rolled parser for mvdan's -// default-expansion behaviour without nounset would re-introduce -// Defect 1 via a typo'd variable name. -func TestResolvedEnv_RealShell_NounsetRegression(t *testing.T) { +// TestResolvedEnv_RealShell_UnsetExpandsEmpty pins Phase 2's lenient +// default: an unset bare $VAR expands to the empty string, matching +// bash. The silent-empty-credential class of bug that motivated Phase +// 1's nounset-on default is already prevented by the pure-function +// error-returning contract of ResolvedEnv, so we no longer rely on +// nounset to catch typo'd variable names. Users who want strict +// behaviour for a required credential opt in per-reference with +// ${VAR:?msg}; see TestResolvedEnv_RealShell_ColonQuestionIsStrict. +func TestResolvedEnv_RealShell_UnsetExpandsEmpty(t *testing.T) { t.Parallel() m := MCPConfig{Env: map[string]string{ - // Intentional typo: user meant $FORGEJO_TOKEN. + // Intentional typo: user meant $FORGEJO_TOKEN. Under Phase 2 + // defaults this expands to "" rather than erroring, matching + // bash's behaviour on bare $VAR. "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN", }} got, err := m.ResolvedEnv(realShellResolver(nil)) - require.Error(t, err, "unset var must not silently expand to empty") + require.NoError(t, err, "unset var must expand to empty, not error") + require.Equal(t, []string{"FORGEJO_ACCESS_TOKEN="}, got) +} + +// TestResolvedEnv_RealShell_ColonQuestionIsStrict pins the opt-in +// strictness contract: ${VAR:?msg} must hard-error when VAR is unset, +// regardless of the global NoUnset toggle. This is the recommended +// mechanism for required credentials under the lenient default, so a +// future refactor that accidentally swallows ${VAR:?...} errors would +// silently ship empty tokens to MCP servers again. +func TestResolvedEnv_RealShell_ColonQuestionIsStrict(t *testing.T) { + t.Parallel() + + m := MCPConfig{Env: map[string]string{ + "FORGEJO_ACCESS_TOKEN": "${FORGJO_TOKEN:?set FORGJO_TOKEN}", + }} + got, err := m.ResolvedEnv(realShellResolver(nil)) + require.Error(t, err, "${VAR:?msg} must error when VAR is unset") require.Nil(t, got) - require.Contains(t, err.Error(), "FORGEJO_ACCESS_TOKEN") - require.Contains(t, err.Error(), "$FORGJO_TOKEN") + // The resolver wraps with the env key and the user-written + // template; the inner shell error carries the :? message so + // users learn which credential is missing and why. + msg := err.Error() + require.Contains(t, msg, "FORGEJO_ACCESS_TOKEN") + require.Contains(t, msg, "${FORGJO_TOKEN:?set FORGJO_TOKEN}") + require.Contains(t, msg, "set FORGJO_TOKEN") } // TestResolvedEnv_RealShell_FailureDetail pins that a failing inner