From 27c2a1ce305349a35849206fca2bf0bd383803e4 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Tue, 10 Mar 2026 12:11:42 -0400 Subject: [PATCH 1/4] Add FlaggedCommand interface so the framework owns flag parsing Introduce FlaggedCommand/FlaggedHandlerFunc to builtins.go: the framework creates a *pflag.FlagSet per invocation, calls the command's MakeFlags factory to register flags and get a bound handler, then runs Parse and error reporting before dispatching with positional args only. Commands no longer contain boilerplate for fs.SetOutput, fs.Parse, or parse-error formatting. Expose type FlagSet = pflag.FlagSet via the builtins package so command files can use *builtins.FlagSet in their factory signature without importing pflag directly (the builtins package is always-allowed in the import allowlist). Add Registrable interface (Register method on both Command and FlaggedCommand) so register_builtins.go can hold both types in one slice. Convert head to FlaggedCommand as the first consumer of the new interface. Co-Authored-By: Claude Sonnet 4.6 --- go.mod | 4 +- interp/builtins/builtins.go | 58 ++++++++++++++++ interp/builtins/head/head.go | 126 +++++++++++++++++------------------ interp/register_builtins.go | 4 +- 4 files changed, 122 insertions(+), 70 deletions(-) diff --git a/go.mod b/go.mod index 244a6f3e..fb149a7b 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,8 @@ module github.com/DataDog/rshell go 1.25.6 require ( + github.com/spf13/cobra v1.10.2 + github.com/spf13/pflag v1.0.9 github.com/stretchr/testify v1.11.1 gopkg.in/yaml.v3 v3.0.1 mvdan.cc/sh/v3 v3.12.0 @@ -12,6 +14,4 @@ require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/spf13/cobra v1.10.2 // indirect - github.com/spf13/pflag v1.0.9 // indirect ) diff --git a/interp/builtins/builtins.go b/interp/builtins/builtins.go index 44fbd46f..10411d5e 100644 --- a/interp/builtins/builtins.go +++ b/interp/builtins/builtins.go @@ -10,11 +10,30 @@ import ( "fmt" "io" "os" + + "github.com/spf13/pflag" ) +// FlagSet is a type alias for pflag.FlagSet. Builtin command files that use +// FlaggedCommand receive a *FlagSet from the framework without needing to +// import pflag directly (the builtins package is always allowed by the import +// allowlist, so builtins.FlagSet is accessible in command implementation files). +type FlagSet = pflag.FlagSet + // HandlerFunc is the signature for a builtin command implementation. type HandlerFunc func(ctx context.Context, callCtx *CallContext, args []string) Result +// BoundHandlerFunc is the run function returned by a FlaggedHandlerFunc. args +// contains only the positional (non-flag) arguments after the framework has +// parsed the FlagSet. +type BoundHandlerFunc func(ctx context.Context, callCtx *CallContext, args []string) Result + +// FlaggedHandlerFunc is called once per invocation to register flags on the +// framework-provided FlagSet and return a BoundHandlerFunc whose flag variables +// are captured by closure. The framework calls Parse then invokes the bound +// handler with the remaining positional arguments. +type FlaggedHandlerFunc func(fs *FlagSet) BoundHandlerFunc + // CallContext provides the capabilities available to builtin commands. // It is created by the Runner for each builtin invocation. type CallContext struct { @@ -72,6 +91,28 @@ type Command struct { Run HandlerFunc } +// FlaggedCommand pairs a builtin name with a flag-declaring factory. Use this +// instead of Command for builtins that accept flags. The framework creates a +// *FlagSet, passes it to MakeFlags so the command can register its flags, then +// calls Parse and invokes the returned handler with positional arguments only. +type FlaggedCommand struct { + Name string + MakeFlags FlaggedHandlerFunc +} + +// Registrable is implemented by both Command and FlaggedCommand so they can be +// collected in a single slice and registered uniformly. +type Registrable interface { + Register() +} + +// Register adds the Command to the builtin registry. +func (c Command) Register() { Register(c.Name, c.Run) } + +// Register adds the FlaggedCommand to the builtin registry via the flagged +// adapter, which handles FlagSet creation, Parse, and error reporting. +func (c FlaggedCommand) Register() { RegisterFlagged(c.Name, c.MakeFlags) } + var registry = map[string]HandlerFunc{} // Register adds a builtin command to the registry. @@ -83,6 +124,23 @@ func Register(name string, fn HandlerFunc) { registry[name] = fn } +// RegisterFlagged registers a FlaggedHandlerFunc under name. For each +// invocation the adapter creates a fresh *FlagSet, calls factory to register +// flags and obtain the bound handler, parses the raw args, writes any error to +// stderr (exit 1), and then calls the handler with the positional args. +func RegisterFlagged(name string, factory FlaggedHandlerFunc) { + Register(name, func(ctx context.Context, callCtx *CallContext, args []string) Result { + fs := pflag.NewFlagSet(name, pflag.ContinueOnError) + fs.SetOutput(io.Discard) // handler formats errors itself + handler := factory(fs) + if err := fs.Parse(args); err != nil { + callCtx.Errf("%s: %v\n", name, err) + return Result{Code: 1} + } + return handler(ctx, callCtx, fs.Args()) + }) +} + // Lookup returns the handler for a builtin command. func Lookup(name string) (HandlerFunc, bool) { fn, ok := registry[name] diff --git a/interp/builtins/head/head.go b/interp/builtins/head/head.go index 6fc13881..040504bf 100644 --- a/interp/builtins/head/head.go +++ b/interp/builtins/head/head.go @@ -56,13 +56,11 @@ import ( "os" "strconv" - "github.com/spf13/pflag" - "github.com/DataDog/rshell/interp/builtins" ) // Cmd is the head builtin command descriptor. -var Cmd = builtins.Command{Name: "head", Run: run} +var Cmd = builtins.FlaggedCommand{Name: "head", MakeFlags: registerFlags} // MaxCount is the maximum accepted line or byte count. Values above this // are clamped. This prevents huge theoretical allocations while remaining @@ -73,10 +71,10 @@ const MaxCount = 1<<31 - 1 // 2 147 483 647 // longer than this are reported as an error instead of being buffered. const MaxLineBytes = 1 << 20 // 1 MiB -func run(ctx context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { - fs := pflag.NewFlagSet("head", pflag.ContinueOnError) - fs.SetOutput(io.Discard) - +// registerFlags registers all head flags on the framework-provided FlagSet and +// returns a bound handler whose flag variables are captured by closure. The +// framework calls Parse and passes positional arguments to the handler. +func registerFlags(fs *builtins.FlagSet) builtins.BoundHandlerFunc { help := fs.BoolP("help", "h", false, "print usage and exit") quiet := fs.BoolP("quiet", "q", false, "never print file name headers") _ = fs.Bool("silent", false, "alias for --quiet") @@ -92,76 +90,72 @@ func run(ctx context.Context, callCtx *builtins.CallContext, args []string) buil fs.VarP(linesFlag, "lines", "n", "print the first N lines instead of the first 10") fs.VarP(bytesFlag, "bytes", "c", "print the first N bytes instead of lines") - if err := fs.Parse(args); err != nil { - callCtx.Errf("head: %v\n", err) - return builtins.Result{Code: 1} - } - - if *help { - callCtx.Out("Usage: head [OPTION]... [FILE]...\n") - callCtx.Out("Print the first 10 lines of each FILE to standard output.\n") - callCtx.Out("With no FILE, or when FILE is -, 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 - } + return func(ctx context.Context, callCtx *builtins.CallContext, files []string) builtins.Result { + if *help { + callCtx.Out("Usage: head [OPTION]... [FILE]...\n") + callCtx.Out("Print the first 10 lines of each FILE to standard output.\n") + callCtx.Out("With no FILE, or when FILE is -, read standard input.\n\n") + fs.SetOutput(callCtx.Stdout) + fs.PrintDefaults() + return builtins.Result{} + } - // Bytes mode wins if -c/--bytes was parsed after -n/--lines. When neither - // is set both pos fields are 0 (false → line mode). When only one is set - // the other stays 0, so the comparison selects correctly. - useBytesMode := bytesFlag.pos > linesFlag.pos + // --silent is an alias for --quiet. + if fs.Changed("silent") { + *quiet = true + } - // Parse the count for the chosen mode. - countStr := linesFlag.val - modeLabel := "lines" - if useBytesMode { - countStr = bytesFlag.val - modeLabel = "bytes" - } + // Bytes mode wins if -c/--bytes was parsed after -n/--lines. When neither + // is set both pos fields are 0 (false → line mode). When only one is set + // the other stays 0, so the comparison selects correctly. + useBytesMode := bytesFlag.pos > linesFlag.pos - count, ok := parseCount(countStr) - if !ok { - callCtx.Errf("head: invalid number of %s: %q\n", modeLabel, countStr) - return builtins.Result{Code: 1} - } + // Parse the count for the chosen mode. + countStr := linesFlag.val + modeLabel := "lines" + if useBytesMode { + countStr = bytesFlag.val + modeLabel = "bytes" + } - // Collect file arguments; default to stdin. - files := fs.Args() - if len(files) == 0 { - files = []string{"-"} - } + count, ok := parseCount(countStr) + if !ok { + callCtx.Errf("head: invalid number of %s: %q\n", modeLabel, countStr) + return builtins.Result{Code: 1} + } - // Header printing: on by default for multiple files, suppressed by -q, - // forced for a single file by -v. - printHeaders := len(files) > 1 || *verbose - if *quiet { - printHeaders = false - } + // Default to stdin when no file arguments were given. + if len(files) == 0 { + files = []string{"-"} + } - var failed bool - for i, file := range files { - if ctx.Err() != nil { - break + // Header printing: on by default for multiple files, suppressed by -q, + // forced for a single file by -v. + printHeaders := len(files) > 1 || *verbose + if *quiet { + printHeaders = false } - if err := processFile(ctx, callCtx, file, i, printHeaders, useBytesMode, count); err != nil { - name := file - if file == "-" { - name = "standard input" + + var failed bool + for i, file := range files { + if ctx.Err() != nil { + break + } + if err := processFile(ctx, callCtx, file, i, printHeaders, useBytesMode, count); err != nil { + name := file + if file == "-" { + name = "standard input" + } + callCtx.Errf("head: %s: %s\n", name, callCtx.PortableErr(err)) + failed = true } - callCtx.Errf("head: %s: %s\n", name, callCtx.PortableErr(err)) - failed = true } - } - if failed { - return builtins.Result{Code: 1} + if failed { + return builtins.Result{Code: 1} + } + return builtins.Result{} } - return builtins.Result{} } // processFile opens and processes one file (or stdin for "-"). diff --git a/interp/register_builtins.go b/interp/register_builtins.go index f6ff973d..20a3e00c 100644 --- a/interp/register_builtins.go +++ b/interp/register_builtins.go @@ -23,7 +23,7 @@ var registerOnce sync.Once func registerBuiltins() { registerOnce.Do(func() { - for _, cmd := range []builtins.Command{ + for _, cmd := range []builtins.Registrable{ breakcmd.Cmd, cat.Cmd, continuecmd.Cmd, @@ -33,7 +33,7 @@ func registerBuiltins() { head.Cmd, truecmd.Cmd, } { - builtins.Register(cmd.Name, cmd.Run) + cmd.Register() } }) } From a78d582e025334e52185b9a3e91d0325e3ec1a5d Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Tue, 10 Mar 2026 12:21:34 -0400 Subject: [PATCH 2/4] Consolidate builtin command interface to a single Command type Replace the parallel Command/FlaggedCommand/HandlerFunc/BoundHandlerFunc/ FlaggedHandlerFunc/Registrable surface with one type each: Command { Name string; MakeFlags func(*FlagSet) HandlerFunc } HandlerFunc func(ctx, callCtx, args) Result Every command declares its flags via MakeFlags. The framework creates a fresh *FlagSet per invocation, calls MakeFlags to register flags and get the bound handler, then parses raw args before calling the handler with positional args. Commands with no flags (echo, cat, break, continue, true, false, exit) use the NoFlags helper which wraps a plain HandlerFunc. The framework detects that no flags were registered via fs.HasFlags() and skips parsing entirely, so flag-shaped literals like "echo -n hello" are passed through unchanged. register_builtins.go now iterates a single []builtins.Command. Co-Authored-By: Claude Sonnet 4.6 --- interp/builtins/break/break.go | 2 +- interp/builtins/builtins.go | 109 +++++++++++---------------- interp/builtins/cat/cat.go | 2 +- interp/builtins/continue/continue.go | 2 +- interp/builtins/echo/echo.go | 2 +- interp/builtins/exit/exit.go | 2 +- interp/builtins/false/false.go | 2 +- interp/builtins/head/head.go | 4 +- interp/builtins/true/true.go | 2 +- interp/register_builtins.go | 2 +- 10 files changed, 55 insertions(+), 74 deletions(-) diff --git a/interp/builtins/break/break.go b/interp/builtins/break/break.go index 05e9de23..0fef1c97 100644 --- a/interp/builtins/break/break.go +++ b/interp/builtins/break/break.go @@ -26,7 +26,7 @@ import ( ) // Cmd is the break builtin command descriptor. -var Cmd = builtins.Command{Name: "break", Run: run} +var Cmd = builtins.Command{Name: "break", MakeFlags: builtins.NoFlags(run)} func run(_ context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { return loopctl.LoopControl(callCtx, "break", args) diff --git a/interp/builtins/builtins.go b/interp/builtins/builtins.go index 10411d5e..c7c93caf 100644 --- a/interp/builtins/builtins.go +++ b/interp/builtins/builtins.go @@ -14,25 +14,55 @@ import ( "github.com/spf13/pflag" ) -// FlagSet is a type alias for pflag.FlagSet. Builtin command files that use -// FlaggedCommand receive a *FlagSet from the framework without needing to -// import pflag directly (the builtins package is always allowed by the import -// allowlist, so builtins.FlagSet is accessible in command implementation files). +// FlagSet is a type alias for pflag.FlagSet. Command files receive a *FlagSet +// from the framework without needing to import pflag directly (the builtins +// package is always allowed by the import allowlist). type FlagSet = pflag.FlagSet -// HandlerFunc is the signature for a builtin command implementation. +// HandlerFunc is the bound handler called by the framework after flags are +// parsed. args contains only the positional (non-flag) arguments. type HandlerFunc func(ctx context.Context, callCtx *CallContext, args []string) Result -// BoundHandlerFunc is the run function returned by a FlaggedHandlerFunc. args -// contains only the positional (non-flag) arguments after the framework has -// parsed the FlagSet. -type BoundHandlerFunc func(ctx context.Context, callCtx *CallContext, args []string) Result +// Command pairs a builtin name with its flag-declaring factory. MakeFlags +// registers any flags on the provided FlagSet and returns the bound handler. +// Commands that accept no flags may ignore fs via NoFlags. +type Command struct { + Name string + MakeFlags func(*FlagSet) HandlerFunc +} -// FlaggedHandlerFunc is called once per invocation to register flags on the -// framework-provided FlagSet and return a BoundHandlerFunc whose flag variables -// are captured by closure. The framework calls Parse then invokes the bound -// handler with the remaining positional arguments. -type FlaggedHandlerFunc func(fs *FlagSet) BoundHandlerFunc +// NoFlags wraps a HandlerFunc in the MakeFlags format for commands that +// declare no flags. +func NoFlags(fn HandlerFunc) func(*FlagSet) HandlerFunc { + return func(_ *FlagSet) HandlerFunc { return fn } +} + +// Register adds the Command to the builtin registry. For each invocation the +// framework creates a fresh *FlagSet, passes it to MakeFlags so the command +// can register its flags, parses the raw args, writes any error to stderr +// (exit 1), and then calls the bound handler with positional args only. +// +// If MakeFlags registers no flags (e.g. via NoFlags), the framework skips +// parsing entirely and passes all raw args to the handler unchanged. This +// lets commands like echo treat flag-shaped literals (e.g. -n) correctly. +func (c Command) Register() { + name := c.Name + factory := c.MakeFlags + addToRegistry(name, func(ctx context.Context, callCtx *CallContext, args []string) Result { + fs := pflag.NewFlagSet(name, pflag.ContinueOnError) + fs.SetOutput(io.Discard) // handler formats errors itself + handler := factory(fs) + if !fs.HasFlags() { + // No flags declared: pass all args through unchanged. + return handler(ctx, callCtx, args) + } + if err := fs.Parse(args); err != nil { + callCtx.Errf("%s: %v\n", name, err) + return Result{Code: 1} + } + return handler(ctx, callCtx, fs.Args()) + }) +} // CallContext provides the capabilities available to builtin commands. // It is created by the Runner for each builtin invocation. @@ -84,66 +114,17 @@ type Result struct { ContinueN int } -// Command pairs a builtin name with its handler, used for explicit -// registration in the all package. -type Command struct { - Name string - Run HandlerFunc -} - -// FlaggedCommand pairs a builtin name with a flag-declaring factory. Use this -// instead of Command for builtins that accept flags. The framework creates a -// *FlagSet, passes it to MakeFlags so the command can register its flags, then -// calls Parse and invokes the returned handler with positional arguments only. -type FlaggedCommand struct { - Name string - MakeFlags FlaggedHandlerFunc -} - -// Registrable is implemented by both Command and FlaggedCommand so they can be -// collected in a single slice and registered uniformly. -type Registrable interface { - Register() -} - -// Register adds the Command to the builtin registry. -func (c Command) Register() { Register(c.Name, c.Run) } - -// Register adds the FlaggedCommand to the builtin registry via the flagged -// adapter, which handles FlagSet creation, Parse, and error reporting. -func (c FlaggedCommand) Register() { RegisterFlagged(c.Name, c.MakeFlags) } - var registry = map[string]HandlerFunc{} -// Register adds a builtin command to the registry. -// It panics if name is already registered, catching duplicate registrations at startup. -func Register(name string, fn HandlerFunc) { +func addToRegistry(name string, fn HandlerFunc) { if _, exists := registry[name]; exists { panic("builtin already registered: " + name) } registry[name] = fn } -// RegisterFlagged registers a FlaggedHandlerFunc under name. For each -// invocation the adapter creates a fresh *FlagSet, calls factory to register -// flags and obtain the bound handler, parses the raw args, writes any error to -// stderr (exit 1), and then calls the handler with the positional args. -func RegisterFlagged(name string, factory FlaggedHandlerFunc) { - Register(name, func(ctx context.Context, callCtx *CallContext, args []string) Result { - fs := pflag.NewFlagSet(name, pflag.ContinueOnError) - fs.SetOutput(io.Discard) // handler formats errors itself - handler := factory(fs) - if err := fs.Parse(args); err != nil { - callCtx.Errf("%s: %v\n", name, err) - return Result{Code: 1} - } - return handler(ctx, callCtx, fs.Args()) - }) -} - // Lookup returns the handler for a builtin command. func Lookup(name string) (HandlerFunc, bool) { fn, ok := registry[name] return fn, ok } - diff --git a/interp/builtins/cat/cat.go b/interp/builtins/cat/cat.go index 2a759e22..0c67cdca 100644 --- a/interp/builtins/cat/cat.go +++ b/interp/builtins/cat/cat.go @@ -27,7 +27,7 @@ import ( ) // Cmd is the cat builtin command descriptor. -var Cmd = builtins.Command{Name: "cat", Run: run} +var Cmd = builtins.Command{Name: "cat", MakeFlags: builtins.NoFlags(run)} func run(ctx context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { if len(args) == 0 { diff --git a/interp/builtins/continue/continue.go b/interp/builtins/continue/continue.go index 1a9bd5b7..e08b64c3 100644 --- a/interp/builtins/continue/continue.go +++ b/interp/builtins/continue/continue.go @@ -26,7 +26,7 @@ import ( ) // Cmd is the continue builtin command descriptor. -var Cmd = builtins.Command{Name: "continue", Run: run} +var Cmd = builtins.Command{Name: "continue", MakeFlags: builtins.NoFlags(run)} func run(_ context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { return loopctl.LoopControl(callCtx, "continue", args) diff --git a/interp/builtins/echo/echo.go b/interp/builtins/echo/echo.go index aec00a42..c6ab9036 100644 --- a/interp/builtins/echo/echo.go +++ b/interp/builtins/echo/echo.go @@ -24,7 +24,7 @@ import ( ) // Cmd is the echo builtin command descriptor. -var Cmd = builtins.Command{Name: "echo", Run: run} +var Cmd = builtins.Command{Name: "echo", MakeFlags: builtins.NoFlags(run)} func run(_ context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { for i, arg := range args { diff --git a/interp/builtins/exit/exit.go b/interp/builtins/exit/exit.go index e92773f1..23df4334 100644 --- a/interp/builtins/exit/exit.go +++ b/interp/builtins/exit/exit.go @@ -28,7 +28,7 @@ import ( ) // Cmd is the exit builtin command descriptor. -var Cmd = builtins.Command{Name: "exit", Run: run} +var Cmd = builtins.Command{Name: "exit", MakeFlags: builtins.NoFlags(run)} func run(_ context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { var r builtins.Result diff --git a/interp/builtins/false/false.go b/interp/builtins/false/false.go index 5da47962..d301ee47 100644 --- a/interp/builtins/false/false.go +++ b/interp/builtins/false/false.go @@ -23,7 +23,7 @@ import ( ) // Cmd is the false builtin command descriptor. -var Cmd = builtins.Command{Name: "false", Run: run} +var Cmd = builtins.Command{Name: "false", MakeFlags: builtins.NoFlags(run)} func run(_ context.Context, _ *builtins.CallContext, _ []string) builtins.Result { return builtins.Result{Code: 1} diff --git a/interp/builtins/head/head.go b/interp/builtins/head/head.go index 040504bf..103a57d6 100644 --- a/interp/builtins/head/head.go +++ b/interp/builtins/head/head.go @@ -60,7 +60,7 @@ import ( ) // Cmd is the head builtin command descriptor. -var Cmd = builtins.FlaggedCommand{Name: "head", MakeFlags: registerFlags} +var Cmd = builtins.Command{Name: "head", MakeFlags: registerFlags} // MaxCount is the maximum accepted line or byte count. Values above this // are clamped. This prevents huge theoretical allocations while remaining @@ -74,7 +74,7 @@ const MaxLineBytes = 1 << 20 // 1 MiB // registerFlags registers all head flags on the framework-provided FlagSet and // returns a bound handler whose flag variables are captured by closure. The // framework calls Parse and passes positional arguments to the handler. -func registerFlags(fs *builtins.FlagSet) builtins.BoundHandlerFunc { +func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc { help := fs.BoolP("help", "h", false, "print usage and exit") quiet := fs.BoolP("quiet", "q", false, "never print file name headers") _ = fs.Bool("silent", false, "alias for --quiet") diff --git a/interp/builtins/true/true.go b/interp/builtins/true/true.go index 8908f9df..ce9493b2 100644 --- a/interp/builtins/true/true.go +++ b/interp/builtins/true/true.go @@ -23,7 +23,7 @@ import ( ) // Cmd is the true builtin command descriptor. -var Cmd = builtins.Command{Name: "true", Run: run} +var Cmd = builtins.Command{Name: "true", MakeFlags: builtins.NoFlags(run)} func run(_ context.Context, _ *builtins.CallContext, _ []string) builtins.Result { return builtins.Result{} diff --git a/interp/register_builtins.go b/interp/register_builtins.go index 20a3e00c..c1c7c7a4 100644 --- a/interp/register_builtins.go +++ b/interp/register_builtins.go @@ -23,7 +23,7 @@ var registerOnce sync.Once func registerBuiltins() { registerOnce.Do(func() { - for _, cmd := range []builtins.Registrable{ + for _, cmd := range []builtins.Command{ breakcmd.Cmd, cat.Cmd, continuecmd.Cmd, From 54876099933e6bc22bb436012eae83e6d3e793b5 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Tue, 10 Mar 2026 12:33:16 -0400 Subject: [PATCH 3/4] Remove pflag and io.Discard from builtin import allowlist pflag.ContinueOnError, pflag.NewFlagSet, and io.Discard moved into builtins.go (the framework layer, exempt from the allowlist) as part of the FlaggedCommand consolidation. No command implementation file references these symbols anymore. Co-Authored-By: Claude Sonnet 4.6 --- tests/import_allowlist_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/import_allowlist_test.go b/tests/import_allowlist_test.go index 3ba7053a..96ec3ba1 100644 --- a/tests/import_allowlist_test.go +++ b/tests/import_allowlist_test.go @@ -38,14 +38,8 @@ var builtinAllowedSymbols = []string{ "context.Context", // errors.Is — error comparison; pure function, no I/O. "errors.Is", - // pflag.ContinueOnError — flag parse-error mode constant; no side effects. - "github.com/spf13/pflag.ContinueOnError", - // pflag.NewFlagSet — CLI flag parsing; operates only on string slices, no I/O. - "github.com/spf13/pflag.NewFlagSet", // io.Copy — stream data between reader and writer; builtins receive sandboxed streams. "io.Copy", - // io.Discard — /dev/null writer; discards all data, no side effects. - "io.Discard", // io.EOF — sentinel error value; pure constant. "io.EOF", // io.NopCloser — wraps a Reader with a no-op Close; no side effects. From 487883772d7e834f7e58a0ce39c8b03dfbfdaedb Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Tue, 10 Mar 2026 12:39:02 -0400 Subject: [PATCH 4/4] Fix CI: convert cat and wc to MakeFlags interface Merge from main brought in a reimplemented cat and a new wc command, both using the old Run field and manually calling pflag.NewFlagSet / io.Discard inside their run functions. This broke the build (unknown field Run) and would have failed the import allowlist (pflag.NewFlagSet, pflag.ContinueOnError, and io.Discard were removed from the allowlist in this PR). Convert both to registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc, removing the pflag import from each file. Co-Authored-By: Claude Sonnet 4.6 --- interp/builtins/cat/cat.go | 135 ++++++++++++++--------------- interp/builtins/wc/wc.go | 169 ++++++++++++++++++------------------- 2 files changed, 143 insertions(+), 161 deletions(-) diff --git a/interp/builtins/cat/cat.go b/interp/builtins/cat/cat.go index 6a023a66..5e6101b6 100644 --- a/interp/builtins/cat/cat.go +++ b/interp/builtins/cat/cat.go @@ -71,13 +71,11 @@ import ( "io" "os" - "github.com/spf13/pflag" - "github.com/DataDog/rshell/interp/builtins" ) // Cmd is the cat builtin command descriptor. -var Cmd = builtins.Command{Name: "cat", MakeFlags: builtins.NoFlags(run)} +var Cmd = builtins.Command{Name: "cat", MakeFlags: registerFlags} // MaxLineBytes is the per-line buffer cap for the line scanner. Lines // longer than this are reported as an error instead of being buffered. @@ -90,10 +88,7 @@ const ( lineNumWidth = 6 // GNU cat line-number field width ) -func run(ctx context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { - fs := pflag.NewFlagSet("cat", pflag.ContinueOnError) - fs.SetOutput(io.Discard) - +func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc { help := fs.BoolP("help", "h", false, "print usage and exit") number := fs.BoolP("number", "n", false, "number all output lines") numberNonblank := fs.BoolP("number-nonblank", "b", false, "number non-blank output lines, overrides -n") @@ -106,80 +101,76 @@ func run(ctx context.Context, callCtx *builtins.CallContext, args []string) buil flagT := fs.BoolP("show-nonprinting-tabs", "t", false, "equivalent to -vT") _ = fs.BoolP("unbuffered", "u", false, "ignored") - if err := fs.Parse(args); err != nil { - callCtx.Errf("cat: %v\n", err) - return builtins.Result{Code: 1} - } - - if *help { - callCtx.Out("Usage: cat [OPTION]... [FILE]...\n") - callCtx.Out("Concatenate FILE(s) to standard output.\n") - callCtx.Out("With no FILE, or when FILE is -, read standard input.\n\n") - fs.SetOutput(callCtx.Stdout) - fs.PrintDefaults() - return builtins.Result{} - } - - if *showAll { - *showNonprinting = true - *showEnds = true - *showTabs = true - } - if *flagE { - *showNonprinting = true - *showEnds = true - } - if *flagT { - *showNonprinting = true - *showTabs = true - } - if *numberNonblank { - *number = false - } - - needsLineProcessing := *number || *numberNonblank || *squeezeBlank || - *showEnds || *showTabs || *showNonprinting + return func(ctx context.Context, callCtx *builtins.CallContext, files []string) builtins.Result { + if *help { + callCtx.Out("Usage: cat [OPTION]... [FILE]...\n") + callCtx.Out("Concatenate FILE(s) to standard output.\n") + callCtx.Out("With no FILE, or when FILE is -, read standard input.\n\n") + fs.SetOutput(callCtx.Stdout) + fs.PrintDefaults() + return builtins.Result{} + } - files := fs.Args() - if len(files) == 0 { - files = []string{"-"} - } + if *showAll { + *showNonprinting = true + *showEnds = true + *showTabs = true + } + if *flagE { + *showNonprinting = true + *showEnds = true + } + if *flagT { + *showNonprinting = true + *showTabs = true + } + if *numberNonblank { + *number = false + } - st := &state{ - number: *number, - numberNonblank: *numberNonblank, - squeezeBlank: *squeezeBlank, - showEnds: *showEnds, - showTabs: *showTabs, - showNonprinting: *showNonprinting, - lineNum: 1, - } + needsLineProcessing := *number || *numberNonblank || *squeezeBlank || + *showEnds || *showTabs || *showNonprinting - var failed bool - for _, file := range files { - if ctx.Err() != nil { - break + if len(files) == 0 { + files = []string{"-"} } - var err error - if needsLineProcessing { - err = catLines(ctx, callCtx, file, st) - } else { - err = catRaw(ctx, callCtx, file) + + st := &state{ + number: *number, + numberNonblank: *numberNonblank, + squeezeBlank: *squeezeBlank, + showEnds: *showEnds, + showTabs: *showTabs, + showNonprinting: *showNonprinting, + lineNum: 1, } - if err != nil { - name := file - if file == "-" { - name = "standard input" + + var failed bool + for _, file := range files { + if ctx.Err() != nil { + break + } + var err error + if needsLineProcessing { + err = catLines(ctx, callCtx, file, st) + } else { + err = catRaw(ctx, callCtx, file) + } + if err != nil { + name := file + if file == "-" { + name = "standard input" + } + callCtx.Errf("cat: %s: %s\n", name, callCtx.PortableErr(err)) + failed = true } - callCtx.Errf("cat: %s: %s\n", name, callCtx.PortableErr(err)) - failed = true } - } - if failed { - return builtins.Result{Code: 1} + if failed { + return builtins.Result{Code: 1} + } + return builtins.Result{} } - return builtins.Result{} } type state struct { diff --git a/interp/builtins/wc/wc.go b/interp/builtins/wc/wc.go index d5d3680f..1b71418b 100644 --- a/interp/builtins/wc/wc.go +++ b/interp/builtins/wc/wc.go @@ -63,13 +63,11 @@ import ( "unicode" "unicode/utf8" - "github.com/spf13/pflag" - "github.com/DataDog/rshell/interp/builtins" ) // Cmd is the wc builtin command descriptor. -var Cmd = builtins.Command{Name: "wc", Run: run} +var Cmd = builtins.Command{Name: "wc", MakeFlags: registerFlags} const chunkSize = 32 * 1024 // 32 KiB read buffer const stdinMinWidth = 7 // GNU wc minimum column width for stdin @@ -90,10 +88,7 @@ type options struct { showMaxLineLen bool } -func run(ctx context.Context, callCtx *builtins.CallContext, args []string) builtins.Result { - fs := pflag.NewFlagSet("wc", pflag.ContinueOnError) - fs.SetOutput(io.Discard) - +func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc { help := fs.BoolP("help", "h", false, "print usage and exit") lines := fs.BoolP("lines", "l", false, "print the newline counts") words := fs.BoolP("words", "w", false, "print the word counts") @@ -105,106 +100,102 @@ func run(ctx context.Context, callCtx *builtins.CallContext, args []string) buil // GTFOBins: this flag reads filenames from a file, enabling // data exfiltration in sandboxed environments. - if err := fs.Parse(args); err != nil { - callCtx.Errf("wc: %v\n", err) - return builtins.Result{Code: 1} - } - - if *help { - callCtx.Out("Usage: wc [OPTION]... [FILE]...\n") - callCtx.Out("Print newline, word, and byte counts for each FILE.\n") - callCtx.Out("With no FILE, or when FILE is -, read standard input.\n\n") - fs.SetOutput(callCtx.Stdout) - fs.PrintDefaults() - return builtins.Result{} - } + return func(ctx context.Context, callCtx *builtins.CallContext, files []string) builtins.Result { + if *help { + callCtx.Out("Usage: wc [OPTION]... [FILE]...\n") + callCtx.Out("Print newline, word, and byte counts for each FILE.\n") + callCtx.Out("With no FILE, or when FILE is -, read standard input.\n\n") + fs.SetOutput(callCtx.Stdout) + fs.PrintDefaults() + return builtins.Result{} + } - opts := options{ - showLines: *lines, - showWords: *words, - showBytes: *bytesFlag, - showChars: *chars, - showMaxLineLen: *maxLineLen, - } + opts := options{ + showLines: *lines, + showWords: *words, + showBytes: *bytesFlag, + showChars: *chars, + showMaxLineLen: *maxLineLen, + } - if !opts.showLines && !opts.showWords && !opts.showBytes && !opts.showChars && !opts.showMaxLineLen { - opts.showLines = true - opts.showWords = true - opts.showBytes = true - } + if !opts.showLines && !opts.showWords && !opts.showBytes && !opts.showChars && !opts.showMaxLineLen { + opts.showLines = true + opts.showWords = true + opts.showBytes = true + } - files := fs.Args() - stdinImplicit := len(files) == 0 - if stdinImplicit { - files = []string{"-"} - } + stdinImplicit := len(files) == 0 + if stdinImplicit { + files = []string{"-"} + } - hasStdin := stdinImplicit - if !hasStdin { - for _, f := range files { - if f == "-" { - hasStdin = true - break + hasStdin := stdinImplicit + if !hasStdin { + for _, f := range files { + if f == "-" { + hasStdin = true + break + } } } - } - var total counts - var failed bool - - type fileResult struct { - name string - c counts - } - results := make([]fileResult, 0, len(files)) + var total counts + var failed bool - for _, file := range files { - if ctx.Err() != nil { - break + type fileResult struct { + name string + c counts } - c, err := countFile(ctx, callCtx, file) - if err != nil { - name := file - if file == "-" { - name = "standard input" + results := make([]fileResult, 0, len(files)) + + for _, file := range files { + if ctx.Err() != nil { + break } - callCtx.Errf("wc: %s: %s\n", name, callCtx.PortableErr(err)) - failed = true - if c == (counts{}) { - continue + c, err := countFile(ctx, callCtx, file) + if err != nil { + name := file + if file == "-" { + name = "standard input" + } + callCtx.Errf("wc: %s: %s\n", name, callCtx.PortableErr(err)) + failed = true + if c == (counts{}) { + continue + } + } + results = append(results, fileResult{name: file, c: c}) + total.lines += c.lines + total.words += c.words + total.chars += c.chars + total.bytes += c.bytes + if c.maxLineLen > total.maxLineLen { + total.maxLineLen = c.maxLineLen } } - results = append(results, fileResult{name: file, c: c}) - total.lines += c.lines - total.words += c.words - total.chars += c.chars - total.bytes += c.bytes - if c.maxLineLen > total.maxLineLen { - total.maxLineLen = c.maxLineLen - } - } - width := fieldWidth(total, opts) - if hasStdin && width < stdinMinWidth { - width = stdinMinWidth - } + width := fieldWidth(total, opts) + if hasStdin && width < stdinMinWidth { + width = stdinMinWidth + } - for _, fr := range results { - name := fr.name - if name == "-" && stdinImplicit { - name = "" + for _, fr := range results { + name := fr.name + if name == "-" && stdinImplicit { + name = "" + } + printCounts(callCtx, fr.c, opts, width, name) } - printCounts(callCtx, fr.c, opts, width, name) - } - if len(files) > 1 { - printCounts(callCtx, total, opts, width, "total") - } + if len(files) > 1 { + printCounts(callCtx, total, opts, width, "total") + } - if failed { - return builtins.Result{Code: 1} + if failed { + return builtins.Result{Code: 1} + } + return builtins.Result{} } - return builtins.Result{} } func countFile(ctx context.Context, callCtx *builtins.CallContext, path string) (counts, error) {