git.iko.soy/crush
on main help sign in
shell: switch config value expansion to lenient by default

Missing env vars in crush.json used to fail loudly. They now quietly expand to an empty string, the same as bash. If you want the old strict behavior for a specific value, write it as ${VAR:?message} and Crush will still complain when the variable is not set. Co-Authored-By: Charm Crush <crush@charm.land>

commitabacef933667599de956fc9493166bf9f92c56f6 authorChristian Rocha <christian@rocha.is> dateSun 3 May 2026, 17:02 (6 months ago) parent4068422
2 files changed+84−17
compare with main
unifiedsplit
internal/shell/expand.go+33−8 internal/shell/expand_test.go+51−9
internal/shell/expand.go+33−8
@@ -7,6 +7,7 @@ import (
77 "io"
88 "os"
99 "strings"
10+ "sync/atomic"
1011
1112 "mvdan.cc/sh/v3/expand"
1213 "mvdan.cc/sh/v3/interp"
@@ -18,11 +19,28 @@ import (
1819// to be embedded in a failing inner command.
1920const maxInnerStderrBytes = 512
2021
22+// NoUnset controls whether ExpandValue treats unset variables as an
23+// error. Default false matches bash: $UNSET expands to "". Store true
24+// to re-enable strict mode globally. Not exposed in crush.json; this is
25+// an internal escape hatch in case the lenient default turns out to be
26+// the wrong call.
27+//
28+// Declared atomic because ExpandValue is invoked concurrently (multiple
29+// MCP / LSP / provider loads in flight at startup, hook execution, etc.)
30+// and an unsynchronised read/write pair is a data race under the Go
31+// memory model regardless of test-level happens-before reasoning. The
32+// atomic load on the hot path is negligible against the cost of parsing
33+// and running through mvdan.
34+//
35+// See PLAN.md Phase 2 design decisions #11 and #12 for the full
36+// rationale.
37+var NoUnset atomic.Bool
38+
2139// ExpandValue expands shell-style substitutions in a single config value.
2240//
2341// Supported constructs match the bash tool:
2442//
25−// - $VAR and ${VAR} (unset is an error; see nounset below).
43+// - $VAR and ${VAR}.
2644// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.
2745// - $(command) with full quoting and nesting.
2846// - escaped and quoted strings ("...", '...').
@@ -32,9 +50,11 @@ const maxInnerStderrBytes = 512
3250// - Returns exactly one string. No field splitting, no globbing, no
3351// pathname generation. Multi-word command output is preserved
3452// verbatim; it is never split into multiple values.
35−// - Nounset is on: unset variables produce an error instead of
36−// expanding to the empty string. Use ${VAR:-default} to opt in to
37−// an empty fallback.
53+// - Nounset is off by default, matching bash: unset variables expand
54+// to "". Opt in to strict behaviour per-reference with
55+// ${VAR:?msg}, which errors loudly when VAR is unset regardless of
56+// the global toggle. Flip the global default via
57+// shell.NoUnset.Store(true) as an internal escape hatch.
3858// - Embedded whitespace and newlines in the input are preserved
3959// verbatim. Command substitution strips trailing newlines only
4060// (POSIX), never leading or internal whitespace.
@@ -63,22 +83,27 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error
6383 logger: noopLogger{},
6484 }
6585
86+ strict := NoUnset.Load()
87+
6688 var stderrBuf bytes.Buffer
6789 cfg := &expand.Config{
6890 Env: expand.ListEnviron(env...),
69− NoUnset: true,
91+ NoUnset: strict,
7092 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {
7193 stderrBuf.Reset()
72− runner, rerr := interp.New(
94+ runnerOpts := []interp.RunnerOption{
7395 interp.StdIO(nil, w, &stderrBuf),
7496 interp.Interactive(false),
7597 interp.Env(expand.ListEnviron(env...)),
7698 interp.Dir(s.cwd),
7799 interp.ExecHandlers(s.execHandlers()...),
100+ }
101+ if strict {
78102 // Match the outer NoUnset: an unset $VAR inside
79103 // $(...) is also an error, not a silent empty.
80− interp.Params("-u"),
81− )
104+ runnerOpts = append(runnerOpts, interp.Params("-u"))
105+ }
106+ runner, rerr := interp.New(runnerOpts...)
82107 if rerr != nil {
83108 return rerr
84109 }
internal/shell/expand_test.go+51−9
@@ -105,22 +105,25 @@ func TestExpandValue_Success(t *testing.T) {
105105func TestExpandValue_Errors(t *testing.T) {
106106 t.Parallel()
107107
108− t.Run("unset var is an error, not empty", func(t *testing.T) {
108+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {
109109 t.Parallel()
110− _, err := ExpandValue(t.Context(), "$MISSING", nil)
111− require.Error(t, err)
110+ got, err := ExpandValue(t.Context(), "$MISSING", nil)
111+ require.NoError(t, err)
112+ require.Equal(t, "", got)
112113 })
113114
114− t.Run("unset var inside braces is an error", func(t *testing.T) {
115+ t.Run("unset var inside braces expands to empty", func(t *testing.T) {
115116 t.Parallel()
116− _, err := ExpandValue(t.Context(), "${MISSING}", nil)
117− require.Error(t, err)
117+ got, err := ExpandValue(t.Context(), "${MISSING}", nil)
118+ require.NoError(t, err)
119+ require.Equal(t, "", got)
118120 })
119121
120− t.Run("unset var inside cmdsubst is an error", func(t *testing.T) {
122+ t.Run("unset var inside cmdsubst expands to empty", func(t *testing.T) {
121123 t.Parallel()
122− _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
123− require.Error(t, err)
124+ got, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
125+ require.NoError(t, err)
126+ require.Equal(t, "", got)
124127 })
125128
126129 t.Run("bad syntax returns error", func(t *testing.T) {
@@ -173,6 +176,45 @@ func TestExpandValue_Errors(t *testing.T) {
173176 })
174177}
175178
179+// TestExpandValue_StrictToggle pins the NoUnset escape hatch: when a
180+// caller flips strict mode on, bare $UNSET must error instead of
181+// expanding to the empty string. Must not run in parallel: it mutates
182+// the package-level NoUnset atomic, so a parallel peer observing the
183+// flipped value would break the lenient default other tests assume.
184+func TestExpandValue_StrictToggle(t *testing.T) {
185+ NoUnset.Store(true)
186+ t.Cleanup(func() { NoUnset.Store(false) })
187+
188+ _, err := ExpandValue(t.Context(), "$UNSET", nil)
189+ require.Error(t, err)
190+
191+ _, err = ExpandValue(t.Context(), "${UNSET}", nil)
192+ require.Error(t, err)
193+
194+ _, err = ExpandValue(t.Context(), `$(printf '%s' "$UNSET")`, nil)
195+ require.Error(t, err)
196+}
197+
198+// TestExpandValue_RequiredOptIn pins the per-reference opt-in strict
199+// idiom ${VAR:?msg}: it must error whether or not the global NoUnset
200+// toggle is on, so config authors can mark individual credentials as
201+// required without flipping the global default.
202+func TestExpandValue_RequiredOptIn(t *testing.T) {
203+ t.Parallel()
204+
205+ _, err := ExpandValue(t.Context(), "${REQUIRED:?must be set}", nil)
206+ require.Error(t, err)
207+ require.Contains(t, err.Error(), "must be set")
208+
209+ got, err := ExpandValue(
210+ t.Context(),
211+ "${REQUIRED:?must be set}",
212+ []string{"REQUIRED=ok"},
213+ )
214+ require.NoError(t, err)
215+ require.Equal(t, "ok", got)
216+}
217+
176218func TestSanitizeStderr(t *testing.T) {
177219 t.Parallel()
178220