git.iko.soy/crush
on main help sign in
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 <crush@charm.land>

commitc3cf9622b20df3b47b40ff52d7a23c7d56910616 authorChristian Rocha <christian@rocha.is> dateSun 3 May 2026, 17:02 (6 months ago) parentabacef9
3 files changed+72−19
compare with main
unifiedsplit
internal/config/load_test.go+15−6 internal/config/mcp_resolved_url_test.go+20−3 internal/config/resolve_real_test.go+37−10
internal/config/load_test.go+15−6
@@ -1617,10 +1617,14 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {
16171617 ExtraHeaders: map[string]string{
16181618 // Failing $(...) — inner command exits 1, no stdout.
16191619 "X-Broken": "$(false)",
1620− // Unset var — nounset makes this an error too.
1620+ // Unset var — under Phase 2 lenient nounset this
1621+ // resolves cleanly to "" rather than erroring;
1622+ // Step 10 will rewrite this whole test once the
1623+ // empty-drop rule replaces the log-and-continue
1624+ // loop at load.go:225.
16211625 "X-Missing": "$PROVIDER_HEADER_NEVER_SET",
16221626 // Happy path: must still be resolved, proving the
1623− // failure in the other two did not abort the loop.
1627+ // failure in X-Broken did not abort the loop.
16241628 "X-Static": "kept-literal",
16251629 },
16261630 },
@@ -1640,10 +1644,15 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {
16401644 pc, ok := cfg.Providers.Get("openai")
16411645 require.True(t, ok, "openai provider must still be configured")
16421646
1643− // Literal template preserved for the two failing headers; the
1644− // happy-path header is resolved through the shell (pass-through
1645− // for a literal value).
1647+ // X-Broken still errors on $(false) so its literal template is
1648+ // preserved by the log-and-continue loop. X-Missing resolves
1649+ // cleanly to "" under lenient nounset: the current divergence
1650+ // only kicks in on resolver errors, so a non-erroring empty
1651+ // expansion writes through. The happy-path header is resolved
1652+ // through the shell (pass-through for a literal value). Step 10
1653+ // will flip this whole surface to the MCP contract plus the
1654+ // empty-drop rule from design decision #18.
16461655 require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"])
1647− require.Equal(t, "$PROVIDER_HEADER_NEVER_SET", pc.ExtraHeaders["X-Missing"])
1656+ require.Equal(t, "", pc.ExtraHeaders["X-Missing"])
16481657 require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"])
16491658}
internal/config/mcp_resolved_url_test.go+20−3
@@ -44,14 +44,31 @@ func TestMCPConfig_ResolvedURL(t *testing.T) {
4444 require.Equal(t, "https://mcp.example.com/events", got)
4545 })
4646
47− t.Run("unset var is an error wrapping the template", func(t *testing.T) {
47+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {
4848 t.Parallel()
49+ // Phase 2 defaults to nounset-off: bare $VAR on an unset
50+ // variable expands to "" rather than erroring. Here the
51+ // host collapses to empty, so the caller sees a malformed
52+ // URL rather than a resolver error; that's the expected
53+ // trade-off for making $OPTIONAL-style patterns work, and
54+ // required-credential callers should use ${VAR:?msg}.
4955 m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_MISSING_HOST/api"}
56+ got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
57+ require.NoError(t, err, "unset var must not error under lenient default")
58+ require.Equal(t, "https:///api", got)
59+ })
60+
61+ t.Run("colon-question on unset var errors regardless of toggle", func(t *testing.T) {
62+ t.Parallel()
63+ // ${VAR:?msg} is the opt-in strictness mechanism; it must
64+ // hard-error even with NoUnset off so required credentials
65+ // surface at load time instead of shipping empty-host URLs
66+ // to the transport layer.
67+ m := MCPConfig{Type: MCPHttp, URL: "https://${MCP_MISSING_HOST:?set MCP_MISSING_HOST}/api"}
5068 _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
5169 require.Error(t, err)
5270 require.Contains(t, err.Error(), "url:")
53− require.Contains(t, err.Error(), "$MCP_MISSING_HOST")
54− require.Contains(t, err.Error(), "unbound")
71+ require.Contains(t, err.Error(), "set MCP_MISSING_HOST")
5572 })
5673
5774 t.Run("failing command substitution is an error", func(t *testing.T) {
internal/config/resolve_real_test.go+37−10
@@ -159,23 +159,50 @@ func TestResolvedEnv_RealShell_Deterministic(t *testing.T) {
159159 require.True(t, slices.IsSorted(got), "env slice must be sorted; got %v", got)
160160}
161161
162−// TestResolvedEnv_RealShell_NounsetRegression is the single most
163−// important assertion in this file: an unset variable is an error, not
164−// an empty expansion. Swapping the hand-rolled parser for mvdan's
165−// default-expansion behaviour without nounset would re-introduce
166−// Defect 1 via a typo'd variable name.
167−func TestResolvedEnv_RealShell_NounsetRegression(t *testing.T) {
162+// TestResolvedEnv_RealShell_UnsetExpandsEmpty pins Phase 2's lenient
163+// default: an unset bare $VAR expands to the empty string, matching
164+// bash. The silent-empty-credential class of bug that motivated Phase
165+// 1's nounset-on default is already prevented by the pure-function
166+// error-returning contract of ResolvedEnv, so we no longer rely on
167+// nounset to catch typo'd variable names. Users who want strict
168+// behaviour for a required credential opt in per-reference with
169+// ${VAR:?msg}; see TestResolvedEnv_RealShell_ColonQuestionIsStrict.
170+func TestResolvedEnv_RealShell_UnsetExpandsEmpty(t *testing.T) {
168171 t.Parallel()
169172
170173 m := MCPConfig{Env: map[string]string{
171− // Intentional typo: user meant $FORGEJO_TOKEN.
174+ // Intentional typo: user meant $FORGEJO_TOKEN. Under Phase 2
175+ // defaults this expands to "" rather than erroring, matching
176+ // bash's behaviour on bare $VAR.
172177 "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",
173178 }}
174179 got, err := m.ResolvedEnv(realShellResolver(nil))
175− require.Error(t, err, "unset var must not silently expand to empty")
180+ require.NoError(t, err, "unset var must expand to empty, not error")
181+ require.Equal(t, []string{"FORGEJO_ACCESS_TOKEN="}, got)
182+}
183+
184+// TestResolvedEnv_RealShell_ColonQuestionIsStrict pins the opt-in
185+// strictness contract: ${VAR:?msg} must hard-error when VAR is unset,
186+// regardless of the global NoUnset toggle. This is the recommended
187+// mechanism for required credentials under the lenient default, so a
188+// future refactor that accidentally swallows ${VAR:?...} errors would
189+// silently ship empty tokens to MCP servers again.
190+func TestResolvedEnv_RealShell_ColonQuestionIsStrict(t *testing.T) {
191+ t.Parallel()
192+
193+ m := MCPConfig{Env: map[string]string{
194+ "FORGEJO_ACCESS_TOKEN": "${FORGJO_TOKEN:?set FORGJO_TOKEN}",
195+ }}
196+ got, err := m.ResolvedEnv(realShellResolver(nil))
197+ require.Error(t, err, "${VAR:?msg} must error when VAR is unset")
176198 require.Nil(t, got)
177− require.Contains(t, err.Error(), "FORGEJO_ACCESS_TOKEN")
178− require.Contains(t, err.Error(), "$FORGJO_TOKEN")
199+ // The resolver wraps with the env key and the user-written
200+ // template; the inner shell error carries the :? message so
201+ // users learn which credential is missing and why.
202+ msg := err.Error()
203+ require.Contains(t, msg, "FORGEJO_ACCESS_TOKEN")
204+ require.Contains(t, msg, "${FORGJO_TOKEN:?set FORGJO_TOKEN}")
205+ require.Contains(t, msg, "set FORGJO_TOKEN")
179206}
180207
181208// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner