c3cf962 · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

test: update config tests to match lenient expansion

c3cf962Christian Rocha6 months ago

internal/config/load_test.go+15−6

compare with main

unifiedsplit

@@ -1617,10 +1617,14 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {

1617 ExtraHeaders: map[string]string{

1618 // Failing $(...) — inner command exits 1, no stdout.

1619 "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.

1621 "X-Missing": "$PROVIDER_HEADER_NEVER_SET",

1625 "X-Missing": "$PROVIDER_HEADER_NEVER_SET",

1622 // Happy path: must still be resolved, proving the

1626 // 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.

1624 "X-Static": "kept-literal",

1628 "X-Static": "kept-literal",

1625 },

1629 },

1626 },

1630 },

@@ -1640,10 +1644,15 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {

1640 pc, ok := cfg.Providers.Get("openai")

1644 pc, ok := cfg.Providers.Get("openai")

1641 require.True(t, ok, "openai provider must still be configured")

1645 require.True(t, ok, "openai provider must still be configured")

1642

1646

1643− // Literal template preserved for the two failing headers; the

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

1644− // happy-path header is resolved through the shell (pass-through

1652+ // through the shell (pass-through for a literal value). Step 10

1645− // for a literal value).

1653+ // will flip this whole surface to the MCP contract plus the

1654+ // empty-drop rule from design decision #18.

1646 require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"])

1655 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"])

1648 require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"])

1657 require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"])

1649}

1658}

internal/config/mcp_resolved_url_test.go+20−3

@@ -44,14 +44,31 @@ func TestMCPConfig_ResolvedURL(t *testing.T) {

44 require.Equal(t, "https://mcp.example.com/events", got)

45 })

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) {

48 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}.

49 m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_MISSING_HOST/api"}

55 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"}

50 _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))

68 _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))

51 require.Error(t, err)

69 require.Error(t, err)

52 require.Contains(t, err.Error(), "url:")

70 require.Contains(t, err.Error(), "url:")

53− require.Contains(t, err.Error(), "$MCP_MISSING_HOST")

71+ require.Contains(t, err.Error(), "set MCP_MISSING_HOST")

54− require.Contains(t, err.Error(), "unbound")

55 })

72 })

57 t.Run("failing command substitution is an error", func(t *testing.T) {

74 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) {

159 require.True(t, slices.IsSorted(got), "env slice must be sorted; got %v", got)

160}

161

162−// TestResolvedEnv_RealShell_NounsetRegression is the single most

162+// TestResolvedEnv_RealShell_UnsetExpandsEmpty pins Phase 2's lenient

163−// important assertion in this file: an unset variable is an error, not

163+// default: an unset bare $VAR expands to the empty string, matching

164−// an empty expansion. Swapping the hand-rolled parser for mvdan's

164+// bash. The silent-empty-credential class of bug that motivated Phase

165−// default-expansion behaviour without nounset would re-introduce

165+// 1's nounset-on default is already prevented by the pure-function

166−// Defect 1 via a typo'd variable name.

166+// error-returning contract of ResolvedEnv, so we no longer rely on

167−func TestResolvedEnv_RealShell_NounsetRegression(t *testing.T) {

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) {

168 t.Parallel()

171 t.Parallel()

169

172

170 m := MCPConfig{Env: map[string]string{

173 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.

172 "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",

177 "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",

173 }}

178 }}

174 got, err := m.ResolvedEnv(realShellResolver(nil))

179 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")

176 require.Nil(t, got)

198 require.Nil(t, got)

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()

177− require.Contains(t, err.Error(), "FORGEJO_ACCESS_TOKEN")

203+ require.Contains(t, msg, "FORGEJO_ACCESS_TOKEN")

178− require.Contains(t, err.Error(), "$FORGJO_TOKEN")

204+ require.Contains(t, msg, "${FORGJO_TOKEN:?set FORGJO_TOKEN}")

205+ require.Contains(t, msg, "set FORGJO_TOKEN")

179}

206}

180

207

181// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner

208// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner