From fab17c96631736e962b334e7024c3d22607c6482 Mon Sep 17 00:00:00 2001 From: datadog-bits-staging <264369727+datadog-bits-staging@users.noreply.github.com> Date: Mon, 9 Mar 2026 09:34:18 +0000 Subject: [PATCH 1/2] Replace internal panics with explicit errors Co-authored-by: AlexandreYang <49917914+AlexandreYang@users.noreply.github.com> --- interp/allowed_paths_internal_test.go | 13 +++++++++++++ interp/api.go | 6 +++++- interp/handler.go | 4 +++- interp/vars.go | 19 +++++++++++++++---- 4 files changed, 36 insertions(+), 6 deletions(-) diff --git a/interp/allowed_paths_internal_test.go b/interp/allowed_paths_internal_test.go index 6735bda5..6405219b 100644 --- a/interp/allowed_paths_internal_test.go +++ b/interp/allowed_paths_internal_test.go @@ -142,6 +142,19 @@ func TestRunRecoversPanic(t *testing.T) { assert.Contains(t, err.Error(), "deliberate test panic") } +func TestRunZeroValueRunnerReturnsError(t *testing.T) { + // A zero-value Runner (not created via New) should return an explicit + // error from Run instead of panicking. + var r Runner + parser := syntax.NewParser() + prog, err := parser.Parse(strings.NewReader("echo hi"), "") + require.NoError(t, err) + + err = r.Run(context.Background(), prog) + require.Error(t, err) + assert.Contains(t, err.Error(), "use interp.New to construct a Runner") +} + func TestAllowedPathsExecDefaultBlocksAll(t *testing.T) { dir := t.TempDir() // No AllowedPaths option — default blocks all exec diff --git a/interp/api.go b/interp/api.go index 5af94441..18fb639d 100644 --- a/interp/api.go +++ b/interp/api.go @@ -253,7 +253,8 @@ func StdIO(in io.Reader, out, err io.Writer) RunnerOption { // current directory. func (r *Runner) Reset() { if !r.usedNew { - panic("use interp.New to construct a Runner") + r.exit.fatal(fmt.Errorf("use interp.New to construct a Runner")) + return } if !r.didReset { r.origDir = r.Dir @@ -333,6 +334,9 @@ func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { }() if !r.didReset { r.Reset() + if r.exit.fatalExit { + return r.exit.err + } } r.fillExpandConfig(ctx) if err := validateNode(node); err != nil { diff --git a/interp/handler.go b/interp/handler.go index a0fc711e..608000ba 100644 --- a/interp/handler.go +++ b/interp/handler.go @@ -12,7 +12,9 @@ import ( ) // HandlerCtx returns HandlerContext value stored in ctx. -// It panics if ctx has no HandlerContext stored. +// It panics if ctx has no HandlerContext stored; this indicates a +// programming error by the caller (e.g. passing a context that did not +// originate from the interpreter). func HandlerCtx(ctx context.Context) HandlerContext { hc, ok := ctx.Value(handlerCtxKey{}).(HandlerContext) if !ok { diff --git a/interp/vars.go b/interp/vars.go index c17c73a3..2a7527da 100644 --- a/interp/vars.go +++ b/interp/vars.go @@ -85,9 +85,17 @@ func (o *overlayEnviron) Each(f func(name string, vr expand.Variable) bool) { } } +// internalErrorf records an internal assertion failure as a fatal error. +// Use this for conditions that should be unreachable (e.g. invariants +// enforced by AST validation). +func (r *Runner) internalErrorf(format string, a ...any) { + r.exit.fatal(fmt.Errorf("internal error: "+format, a...)) +} + func (r *Runner) lookupVar(name string) expand.Variable { if name == "" { - panic("variable name must not be empty") + r.internalErrorf("variable name must not be empty") + return expand.Variable{} } // Only $? is supported as a special variable in safe-shell. if name == "?" { @@ -125,7 +133,8 @@ func (r *Runner) setVar(name string, vr expand.Variable) { // blocked by the AST validator, so we only handle simple string assignment. func (r *Runner) setVarWithIndex(prev expand.Variable, name string, index syntax.ArithmExpr, vr expand.Variable) { if index != nil { - panic("setVarWithIndex: index should have been rejected by AST validation") + r.internalErrorf("setVarWithIndex: index should have been rejected by AST validation") + return } prev.Set = true if name2, var2 := prev.Resolve(r.writeEnv); name2 != "" { @@ -142,10 +151,12 @@ func (r *Runner) setVarWithIndex(prev expand.Variable, name string, index syntax func (r *Runner) assignVal(prev expand.Variable, as *syntax.Assign, _ string) expand.Variable { prev.Set = true if as.Append { - panic("assignVal: append should have been rejected by AST validation") + r.internalErrorf("assignVal: append should have been rejected by AST validation") + return expand.Variable{} } if as.Array != nil { - panic("assignVal: array assignment should have been rejected by AST validation") + r.internalErrorf("assignVal: array assignment should have been rejected by AST validation") + return expand.Variable{} } if as.Value != nil { prev.Kind = expand.String From f2dee7ac56aa31d7977ad1a536fcb081d9adb25a Mon Sep 17 00:00:00 2001 From: datadog-bits-staging <264369727+datadog-bits-staging@users.noreply.github.com> Date: Mon, 9 Mar 2026 09:36:58 +0000 Subject: [PATCH 2/2] Add test coverage for internal error paths Co-authored-by: AlexandreYang <49917914+AlexandreYang@users.noreply.github.com> --- interp/internal_errors_test.go | 131 +++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 interp/internal_errors_test.go diff --git a/interp/internal_errors_test.go b/interp/internal_errors_test.go new file mode 100644 index 00000000..f8c3aad3 --- /dev/null +++ b/interp/internal_errors_test.go @@ -0,0 +1,131 @@ +package interp + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "mvdan.cc/sh/v3/expand" + "mvdan.cc/sh/v3/syntax" +) + +// newResetRunner creates a Runner via New and calls Reset so that internal +// state (writeEnv, etc.) is fully initialised for unit-level tests. +func newResetRunner(t *testing.T) *Runner { + t.Helper() + r, err := New() + require.NoError(t, err) + t.Cleanup(func() { r.Close() }) + r.Reset() + return r +} + +func TestInternalErrorf(t *testing.T) { + r := newResetRunner(t) + + r.internalErrorf("something went wrong: %s", "details") + + require.True(t, r.exit.fatalExit) + assert.Contains(t, r.exit.err.Error(), "internal error: something went wrong: details") +} + +func TestInternalErrorfIdempotent(t *testing.T) { + r := newResetRunner(t) + + r.internalErrorf("first error") + r.internalErrorf("second error") + + assert.Contains(t, r.exit.err.Error(), "first error", + "exit.fatal keeps the first error and ignores subsequent calls") +} + +func TestLookupVarEmptyName(t *testing.T) { + r := newResetRunner(t) + + vr := r.lookupVar("") + + assert.Equal(t, expand.Variable{}, vr, "should return zero Variable") + require.True(t, r.exit.fatalExit) + assert.Contains(t, r.exit.err.Error(), "variable name must not be empty") +} + +func TestSetVarWithIndexNonNilIndex(t *testing.T) { + r := newResetRunner(t) + + // Any non-nil ArithmExpr triggers the invariant check. + idx := &syntax.Word{} + r.setVarWithIndex(expand.Variable{}, "X", idx, expand.Variable{}) + + require.True(t, r.exit.fatalExit) + assert.Contains(t, r.exit.err.Error(), "index should have been rejected by AST validation") +} + +func TestAssignValAppend(t *testing.T) { + r := newResetRunner(t) + + as := &syntax.Assign{Append: true, Name: &syntax.Lit{Value: "X"}} + vr := r.assignVal(expand.Variable{}, as, "") + + assert.Equal(t, expand.Variable{}, vr, "should return zero Variable") + require.True(t, r.exit.fatalExit) + assert.Contains(t, r.exit.err.Error(), "append should have been rejected by AST validation") +} + +func TestAssignValArray(t *testing.T) { + r := newResetRunner(t) + + as := &syntax.Assign{ + Name: &syntax.Lit{Value: "X"}, + Array: &syntax.ArrayExpr{}, + } + vr := r.assignVal(expand.Variable{}, as, "") + + assert.Equal(t, expand.Variable{}, vr, "should return zero Variable") + require.True(t, r.exit.fatalExit) + assert.Contains(t, r.exit.err.Error(), "array assignment should have been rejected by AST validation") +} + +func TestResetZeroValueRunnerSetsFatal(t *testing.T) { + var r Runner + r.Reset() + + require.True(t, r.exit.fatalExit) + assert.Contains(t, r.exit.err.Error(), "use interp.New to construct a Runner") +} + +func TestRunZeroValueRunnerMultipleCalls(t *testing.T) { + // Calling Run repeatedly on a zero-value Runner should consistently + // return an explicit error, not panic. + var r Runner + parser := syntax.NewParser() + prog, err := parser.Parse(strings.NewReader("echo hi"), "") + require.NoError(t, err) + + for i := 0; i < 3; i++ { + err = r.Run(context.Background(), prog) + require.Error(t, err) + assert.Contains(t, err.Error(), "use interp.New to construct a Runner") + } +} + +func TestInternalErrorStopsExecution(t *testing.T) { + // After an internal error, the runner's stop() check should halt + // further statement execution, surfacing the error via Run. + r := newResetRunner(t) + + // Inject a fatal internal error before running a program. + r.internalErrorf("forced failure") + + parser := syntax.NewParser() + prog, err := parser.Parse(strings.NewReader("echo should_not_run"), "") + require.NoError(t, err) + + // Run resets exit, but the next stmt() call will see exiting=true. + // We verify the pattern by calling stmts directly on a prepared runner. + r.fillExpandConfig(context.Background()) + r.stmts(context.Background(), prog.Stmts) + + assert.True(t, r.exit.fatalExit, "fatal flag should remain set") +}