Add improve-loop skill for systematic shell feature review

cmd/rshell/main.go

@@ -37,10 +37,11 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) int {
 		SilenceErrors: true,
 		Args:          cobra.ArbitraryArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			if script != "" && len(args) > 0 {
+			scriptSet := cmd.Flags().Changed("script")
+			if scriptSet && len(args) > 0 {
 				return fmt.Errorf("cannot use --script with file arguments")
 			}
-			if script == "" && len(args) == 0 {
+			if !scriptSet && len(args) == 0 {
 				return fmt.Errorf("requires either --script or file arguments (use \"-\" for stdin)")
 			}
 
@@ -49,7 +50,7 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) int {
 				paths = strings.Split(allowedPaths, ",")
 			}
 
-			if script != "" {
+			if scriptSet {
 				return execute(cmd.Context(), script, "", paths, stdin, stdout, stderr)
 			}
 
@@ -105,7 +106,9 @@ func execute(ctx context.Context, script, name string, allowedPaths []string, st
 	// Parse.
 	prog, err := syntax.NewParser().Parse(strings.NewReader(script), name)
 	if err != nil {
-		return fmt.Errorf("parse error: %w", err)
+		// Bash returns exit code 2 for syntax/parse errors.
+		fmt.Fprintf(stderr, "%v\n", err)
+		return interp.ExitStatus(2)
 	}
 
 	// Build runner options.

cmd/rshell/main_test.go

@@ -55,15 +55,34 @@ func TestMissingScriptAndFiles(t *testing.T) {
 	assert.Contains(t, stderr, "requires either --script or file arguments")
 }
 
+func TestEmptyScript(t *testing.T) {
+	code, stdout, stderr := runCLI(t, "-s", "")
+	assert.Equal(t, 0, code, "empty script should exit 0 (matching bash -c '')")
+	assert.Empty(t, stdout)
+	assert.Empty(t, stderr)
+}
+
 func TestExitCode(t *testing.T) {
 	code, _, _ := runCLI(t, "-s", `exit 42`)
 	assert.Equal(t, 42, code)
 }
 
 func TestParseError(t *testing.T) {
 	code, _, stderr := runCLI(t, "-s", `echo "unterminated`)
-	assert.NotEqual(t, 0, code)
-	assert.Contains(t, stderr, "parse error")
+	assert.Equal(t, 2, code, "parse errors should return exit code 2 (matching bash)")
+	assert.Contains(t, stderr, "without closing quote")
+}
+
+func TestParseErrorSyntax(t *testing.T) {
+	code, _, stderr := runCLI(t, "-s", `if; then`)
+	assert.Equal(t, 2, code, "syntax errors should return exit code 2 (matching bash)")
+	assert.Contains(t, stderr, "must be followed by")
+}
+
+func TestParseErrorUnclosed(t *testing.T) {
+	code, _, stderr := runCLI(t, "-s", "if true; then\n  echo hello")
+	assert.Equal(t, 2, code, "unclosed blocks should return exit code 2 (matching bash)")
+	assert.Contains(t, stderr, "must end with")
 }
 
 func setupTestFile(t *testing.T) (dir, filePath string) {

interp/builtins/exit/exit.go

@@ -11,13 +11,13 @@
 //
 // Exit the shell with status N. If N is omitted, the exit status is
 // that of the last command executed. If N is not a valid integer, the
-// shell prints an error and exits with status 2.
+// shell prints an error and exits with status 2 (matching bash behavior).
 //
 // Exit codes:
 //
 //	N    The supplied exit status (truncated to uint8).
-//	2    Invalid (non-numeric) argument.
-//	1    Too many arguments.
+//	2    Invalid (non-numeric) argument (shell exits).
+//	1    Too many arguments (shell exits).
 package exit
 
 import (
@@ -41,15 +41,16 @@ func run(_ context.Context, callCtx *builtins.CallContext, args []string) builti
 	case 1:
 		n, err := strconv.Atoi(args[0])
 		if err != nil {
-			callCtx.Errf("invalid exit status code: %q\n", args[0])
+			callCtx.Errf("%s: numeric argument required\n", args[0])
 			r.Code = 2
-			// In bash, exit with invalid args still terminates the shell.
+			// In bash, exit with a non-numeric arg prints an error and
+			// exits the shell with code 2.
 			r.Exiting = true
 			return r
 		}
 		r.Code = uint8(n)
 	default:
-		callCtx.Errf("exit cannot take multiple arguments\n")
+		callCtx.Errf("too many arguments\n")
 		r.Code = 1
 		// In bash, exit with too many args still terminates the shell.
 		r.Exiting = true

interp/builtins/grep/grep.go

@@ -184,7 +184,19 @@ func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc {
 	var patterns patternSlice
 	fs.VarP(&patterns, "regexp", "e", "use PATTERN as the pattern")
 
+	// Help flag (long-only; -h is taken by --no-filename).
+	help := fs.Bool("help", false, "print usage and exit")
+
 	return func(ctx context.Context, callCtx *builtins.CallContext, args []string) builtins.Result {
+		if *help {
+			callCtx.Out("Usage: grep [OPTION]... PATTERN [FILE]...\n")
+			callCtx.Out("Search for PATTERN in each FILE.\n")
+			callCtx.Out("When FILE is -, read standard input. With no FILE, read standard input.\n\n")
+			fs.SetOutput(callCtx.Stdout)
+			fs.PrintDefaults()
+			return builtins.Result{}
+		}
+
 		// --silent is an alias for --quiet.
 		if fs.Changed("silent") {
 			*quiet = true

interp/builtins/internal/loopctl/loopctl.go

@@ -34,7 +34,7 @@ func LoopControl(callCtx *builtins.CallContext, name string, args []string) buil
 		n = parsed
 	default:
 		callCtx.Errf("%s: too many arguments\n", name)
-		return builtins.Result{Code: 1, BreakN: 1}
+		return builtins.Result{Code: 1, Exiting: true}
 	}
 
 	var r builtins.Result

interp/builtins/ls/ls.go

@@ -85,6 +85,7 @@ import (
 	"errors"
 	"fmt"
 	iofs "io/fs"
+	"runtime"
 	"slices"
 	"time"
 
@@ -126,7 +127,19 @@ func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc {
 	offset := fs.Int("offset", 0, "skip first N entries (pagination)")
 	limit := fs.Int("limit", 0, "show at most N entries (capped at MaxDirEntries)")
 
+	// Help flag (long-only; -h is taken by --human-readable).
+	help := fs.Bool("help", false, "print usage and exit")
+
 	return func(ctx context.Context, callCtx *builtins.CallContext, args []string) builtins.Result {
+		if *help {
+			callCtx.Out("Usage: ls [OPTION]... [FILE]...\n")
+			callCtx.Out("List directory contents.\n")
+			callCtx.Out("List information about the FILEs (the current directory by default).\n\n")
+			fs.SetOutput(callCtx.Stdout)
+			fs.PrintDefaults()
+			return builtins.Result{}
+		}
+
 		now := callCtx.Now()
 
 		// Determine the effective sort mode. When both -S and -t are given,
@@ -608,7 +621,8 @@ func joinPath(dir, name string) string {
 	if len(dir) == 0 {
 		return name
 	}
-	if dir[len(dir)-1] == '/' {
+	last := dir[len(dir)-1]
+	if last == '/' || (runtime.GOOS == "windows" && last == '\\') {
 		return dir + name
 	}
 	return dir + "/" + name

interp/builtins/printf/printf.go

@@ -23,7 +23,7 @@
 // Accepted flags:
 //
 //	--help
-//	    Print a usage message to stderr and exit 2.
+//	    Print a usage message to stdout and exit 2.
 //
 // Rejected flags:
 //
@@ -182,7 +182,7 @@ func run(ctx context.Context, callCtx *builtins.CallContext, args []string) buil
 	if len(args) > 0 {
 		switch {
 		case args[0] == "--help":
-			callCtx.Errf("printf: usage: printf [-v var] format [arguments]\n")
+			callCtx.Out("printf: usage: printf [-v var] format [arguments]\n")
 			return builtins.Result{Code: 2}
 		case args[0] == "-v":
 			callCtx.Errf("printf: -v: not supported in restricted shell\n")
@@ -444,11 +444,11 @@ func processSpecifier(callCtx *builtins.CallContext, s string, args []string, ar
 		return false, i, true
 	}
 
-	// Skip C-style length modifiers (l, ll, h, hh, j, t, z, q).
+	// Skip C-style length modifiers (l, ll, h, hh, j, t, z).
 	// Bash accepts and effectively ignores them.
 	for i < len(s) {
 		switch s[i] {
-		case 'l', 'h', 'j', 't', 'z', 'q':
+		case 'l', 'h', 'j', 't', 'z':
 			i++
 			continue
 		}

interp/builtins/printf/printf_test.go

@@ -395,9 +395,9 @@ func TestPrintfRejectedVFlag(t *testing.T) {
 // --- Help ---
 
 func TestPrintfHelp(t *testing.T) {
-	_, stderr, code := cmdRun(t, `printf --help`)
+	stdout, _, code := cmdRun(t, `printf --help`)
 	assert.Equal(t, 2, code)
-	assert.Contains(t, stderr, "printf: usage:")
+	assert.Contains(t, stdout, "printf: usage:")
 }
 
 func TestPrintfHelpShort(t *testing.T) {

interp/internal_errors_test.go

@@ -243,6 +243,13 @@ func TestSubshellBackgroundCopiesEnv(t *testing.T) {
 		"background subshell env should be isolated from parent mutations")
 }
 
+func TestExpandErrUnexpectedCommand(t *testing.T) {
+	r := newResetRunner(t)
+	r.expandErr(expand.UnexpectedCommandError{Node: &syntax.CmdSubst{}})
+	assert.True(t, r.exit.exiting)
+	assert.Equal(t, uint8(1), r.exit.code)
+}
+
 func TestInternalErrorStopsExecution(t *testing.T) {
 	// After an internal error, the runner's stop() check should halt
 	// further statement execution, surfacing the error via Run.

interp/readonly_test.go

@@ -0,0 +1,90 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2026-present Datadog, Inc.
+
+package interp
+
+import (
+	"bytes"
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"mvdan.cc/sh/v3/expand"
+	"mvdan.cc/sh/v3/syntax"
+)
+
+func TestReadonlyVariableBlocksReassignment(t *testing.T) {
+	var stdout, stderr bytes.Buffer
+	r, err := New(
+		StdIO(nil, &stdout, &stderr),
+		Env("RO_VAR=original"),
+	)
+	require.NoError(t, err)
+	t.Cleanup(func() { r.Close() })
+
+	// Mark RO_VAR as readonly via the environment overlay.
+	r.Reset()
+	r.writeEnv.Set("RO_VAR", expand.Variable{
+		Set:      true,
+		Kind:     expand.String,
+		Str:      "original",
+		ReadOnly: true,
+	})
+
+	parser := syntax.NewParser()
+	prog, err := parser.Parse(strings.NewReader("RO_VAR=changed\necho $RO_VAR"), "")
+	require.NoError(t, err)
+
+	r.fillExpandConfig(context.Background())
+	r.stmts(context.Background(), prog.Stmts)
+
+	assert.Contains(t, stderr.String(), "readonly variable",
+		"reassigning a readonly variable should produce an error on stderr")
+	assert.Contains(t, stdout.String(), "original",
+		"readonly variable value should remain unchanged")
+}
+
+func TestReadonlyVariableBlocksUnset(t *testing.T) {
+	r := newResetRunner(t)
+
+	// Set a readonly variable.
+	r.writeEnv.Set("RO_VAR", expand.Variable{
+		Set:      true,
+		Kind:     expand.String,
+		Str:      "protected",
+		ReadOnly: true,
+	})
+
+	// Attempt to unset it by passing an unset Variable.
+	err := r.writeEnv.Set("RO_VAR", expand.Variable{})
+	assert.Error(t, err, "unsetting a readonly variable should return an error")
+	assert.Contains(t, err.Error(), "readonly variable")
+
+	// Verify the variable is still set.
+	vr := r.writeEnv.Get("RO_VAR")
+	assert.Equal(t, "protected", vr.Str, "readonly variable should still hold its value")
+}
+
+func TestReadonlyVariableBlocksKeepValueAttributeChange(t *testing.T) {
+	r := newResetRunner(t)
+
+	// Set a readonly variable.
+	r.writeEnv.Set("RO_VAR", expand.Variable{
+		Set:      true,
+		Kind:     expand.String,
+		Str:      "locked",
+		ReadOnly: true,
+	})
+
+	// Attempt to change attributes via KeepValue.
+	err := r.writeEnv.Set("RO_VAR", expand.Variable{
+		Kind:     expand.KeepValue,
+		Exported: true,
+	})
+	assert.Error(t, err, "modifying attributes on a readonly variable via KeepValue should fail")
+	assert.Contains(t, err.Error(), "readonly variable")
+}

interp/runner_exec.go

@@ -100,12 +100,14 @@ func (r *Runner) cmd(ctx context.Context, cm syntax.Command) {
 			r.setVar(name, vr)
 		}
 
+		defer func() {
+			for _, restore := range restores {
+				r.setVarRestore(restore.name, restore.vr)
+			}
+		}()
 		if r.exit.ok() {
 			r.call(ctx, cm.Args[0].Pos(), fields)
 		}
-		for _, restore := range restores {
-			r.setVarRestore(restore.name, restore.vr)
-		}
 	case *syntax.BinaryCmd:
 		switch cm.Op {
 		case syntax.AndStmt, syntax.OrStmt:

interp/runner_expand.go

@@ -41,6 +41,9 @@ func (r *Runner) expandErr(err error) {
 	fmt.Fprintln(r.stderr, errMsg)
 	switch {
 	case errors.As(err, &expand.UnsetParameterError{}):
+	case errors.As(err, &expand.UnexpectedCommandError{}):
+		// Defense in depth: command substitution is blocked at AST validation,
+		// but if it leaks through, treat it as fatal.
 	case errMsg == "invalid indirect expansion":
 		// TODO: These errors are treated as fatal by bash.
 		// Make the error type reflect that.

interp/vars.go

@@ -60,15 +60,24 @@ func (o *overlayEnviron) Set(name string, vr expand.Variable) error {
 	if o.values == nil {
 		o.values = make(map[string]expand.Variable)
 	}
+	if prev.ReadOnly && vr.Kind != expand.KeepValue {
+		return fmt.Errorf("readonly variable")
+	}
 	if vr.Kind == expand.KeepValue {
+		if prev.ReadOnly {
+			return fmt.Errorf("readonly variable")
+		}
 		vr.Kind = prev.Kind
 		vr.Str = prev.Str
 		vr.List = prev.List
 		vr.Map = prev.Map
-	} else if prev.ReadOnly {
-		return fmt.Errorf("readonly variable")
 	}
 	if !vr.IsSet() { // unsetting
+		// Note: prev.ReadOnly is always false here (guarded by the checks above),
+		// but we keep this as defense-in-depth in case future refactors change the flow.
+		if prev.ReadOnly {
+			return fmt.Errorf("readonly variable")
+		}
 		if prev.Local {
 			vr.Local = true
 			o.values[name] = vr

tests/allowed_symbols_test.go

@@ -98,6 +98,8 @@ var builtinAllowedSymbols = []string{
 	"os.O_RDONLY",
 	// regexp.Compile — compiles a regular expression; pure function, no I/O. Uses RE2 engine (linear-time, no backtracking).
 	"regexp.Compile",
+	// runtime.GOOS — string constant identifying the operating system; pure constant, no I/O.
+	"runtime.GOOS",
 	// regexp.QuoteMeta — escapes all special regex characters in a string; pure function, no I/O.
 	"regexp.QuoteMeta",
 	// regexp.Regexp — compiled regular expression type; no I/O side effects. All matching methods are linear-time (RE2).