Detailed changes
@@ -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"])
}
@@ -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) {
@@ -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