From c3cf9622b20df3b47b40ff52d7a23c7d56910616 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH] test: update config tests to match lenient expansion Updates tests that assumed missing variables in config values were an error. They now expect an empty string, matching the new default. New tests cover ${VAR:?message}, which is the way to still fail loudly when a variable is required. Co-Authored-By: Charm Crush --- internal/config/load_test.go | 21 ++++++++--- internal/config/mcp_resolved_url_test.go | 23 ++++++++++-- internal/config/resolve_real_test.go | 47 +++++++++++++++++++----- 3 files changed, 72 insertions(+), 19 deletions(-) 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