@@ -7,6 +7,7 @@ import (
"io"
"os"
"strings"
+ "sync/atomic"
"mvdan.cc/sh/v3/expand"
"mvdan.cc/sh/v3/interp"
@@ -18,11 +19,28 @@ import (
// to be embedded in a failing inner command.
const maxInnerStderrBytes = 512
+// NoUnset controls whether ExpandValue treats unset variables as an
+// error. Default false matches bash: $UNSET expands to "". Store true
+// to re-enable strict mode globally. Not exposed in crush.json; this is
+// an internal escape hatch in case the lenient default turns out to be
+// the wrong call.
+//
+// Declared atomic because ExpandValue is invoked concurrently (multiple
+// MCP / LSP / provider loads in flight at startup, hook execution, etc.)
+// and an unsynchronised read/write pair is a data race under the Go
+// memory model regardless of test-level happens-before reasoning. The
+// atomic load on the hot path is negligible against the cost of parsing
+// and running through mvdan.
+//
+// See PLAN.md Phase 2 design decisions #11 and #12 for the full
+// rationale.
+var NoUnset atomic.Bool
+
// ExpandValue expands shell-style substitutions in a single config value.
//
// Supported constructs match the bash tool:
//
-// - $VAR and ${VAR} (unset is an error; see nounset below).
+// - $VAR and ${VAR}.
// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.
// - $(command) with full quoting and nesting.
// - escaped and quoted strings ("...", '...').
@@ -32,9 +50,11 @@ const maxInnerStderrBytes = 512
// - Returns exactly one string. No field splitting, no globbing, no
// pathname generation. Multi-word command output is preserved
// verbatim; it is never split into multiple values.
-// - Nounset is on: unset variables produce an error instead of
-// expanding to the empty string. Use ${VAR:-default} to opt in to
-// an empty fallback.
+// - Nounset is off by default, matching bash: unset variables expand
+// to "". Opt in to strict behaviour per-reference with
+// ${VAR:?msg}, which errors loudly when VAR is unset regardless of
+// the global toggle. Flip the global default via
+// shell.NoUnset.Store(true) as an internal escape hatch.
// - Embedded whitespace and newlines in the input are preserved
// verbatim. Command substitution strips trailing newlines only
// (POSIX), never leading or internal whitespace.
@@ -63,22 +83,27 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error
logger: noopLogger{},
}
+ strict := NoUnset.Load()
+
var stderrBuf bytes.Buffer
cfg := &expand.Config{
Env: expand.ListEnviron(env...),
- NoUnset: true,
+ NoUnset: strict,
CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {
stderrBuf.Reset()
- runner, rerr := interp.New(
+ runnerOpts := []interp.RunnerOption{
interp.StdIO(nil, w, &stderrBuf),
interp.Interactive(false),
interp.Env(expand.ListEnviron(env...)),
interp.Dir(s.cwd),
interp.ExecHandlers(s.execHandlers()...),
+ }
+ if strict {
// Match the outer NoUnset: an unset $VAR inside
// $(...) is also an error, not a silent empty.
- interp.Params("-u"),
- )
+ runnerOpts = append(runnerOpts, interp.Params("-u"))
+ }
+ runner, rerr := interp.New(runnerOpts...)
if rerr != nil {
return rerr
}
@@ -105,22 +105,25 @@ func TestExpandValue_Success(t *testing.T) {
func TestExpandValue_Errors(t *testing.T) {
t.Parallel()
- t.Run("unset var is an error, not empty", func(t *testing.T) {
+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {
t.Parallel()
- _, err := ExpandValue(t.Context(), "$MISSING", nil)
- require.Error(t, err)
+ got, err := ExpandValue(t.Context(), "$MISSING", nil)
+ require.NoError(t, err)
+ require.Equal(t, "", got)
})
- t.Run("unset var inside braces is an error", func(t *testing.T) {
+ t.Run("unset var inside braces expands to empty", func(t *testing.T) {
t.Parallel()
- _, err := ExpandValue(t.Context(), "${MISSING}", nil)
- require.Error(t, err)
+ got, err := ExpandValue(t.Context(), "${MISSING}", nil)
+ require.NoError(t, err)
+ require.Equal(t, "", got)
})
- t.Run("unset var inside cmdsubst is an error", func(t *testing.T) {
+ t.Run("unset var inside cmdsubst expands to empty", func(t *testing.T) {
t.Parallel()
- _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
- require.Error(t, err)
+ got, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
+ require.NoError(t, err)
+ require.Equal(t, "", got)
})
t.Run("bad syntax returns error", func(t *testing.T) {
@@ -173,6 +176,45 @@ func TestExpandValue_Errors(t *testing.T) {
})
}
+// TestExpandValue_StrictToggle pins the NoUnset escape hatch: when a
+// caller flips strict mode on, bare $UNSET must error instead of
+// expanding to the empty string. Must not run in parallel: it mutates
+// the package-level NoUnset atomic, so a parallel peer observing the
+// flipped value would break the lenient default other tests assume.
+func TestExpandValue_StrictToggle(t *testing.T) {
+ NoUnset.Store(true)
+ t.Cleanup(func() { NoUnset.Store(false) })
+
+ _, err := ExpandValue(t.Context(), "$UNSET", nil)
+ require.Error(t, err)
+
+ _, err = ExpandValue(t.Context(), "${UNSET}", nil)
+ require.Error(t, err)
+
+ _, err = ExpandValue(t.Context(), `$(printf '%s' "$UNSET")`, nil)
+ require.Error(t, err)
+}
+
+// TestExpandValue_RequiredOptIn pins the per-reference opt-in strict
+// idiom ${VAR:?msg}: it must error whether or not the global NoUnset
+// toggle is on, so config authors can mark individual credentials as
+// required without flipping the global default.
+func TestExpandValue_RequiredOptIn(t *testing.T) {
+ t.Parallel()
+
+ _, err := ExpandValue(t.Context(), "${REQUIRED:?must be set}", nil)
+ require.Error(t, err)
+ require.Contains(t, err.Error(), "must be set")
+
+ got, err := ExpandValue(
+ t.Context(),
+ "${REQUIRED:?must be set}",
+ []string{"REQUIRED=ok"},
+ )
+ require.NoError(t, err)
+ require.Equal(t, "ok", got)
+}
+
func TestSanitizeStderr(t *testing.T) {
t.Parallel()