diff --git a/docs/adr/0001-scriptrunner-seam.md b/docs/adr/0001-scriptrunner-seam.md new file mode 100644 index 00000000..74ce5c19 --- /dev/null +++ b/docs/adr/0001-scriptrunner-seam.md @@ -0,0 +1,30 @@ +# ADR-0001 — Introduce ScriptRunner seam in executor + +**Date:** 2026-06-14 +**Status:** Accepted + +## Context + +`internal/executor/` orchestrates the full lifecycle of a **Project command** — docopt parsing, **Dependency chain** resolution, **Environment** layering, **Init script**, **After script**, and **Persisted checksum** — but it had no testable boundary between that orchestration and OS process spawning. Every function that needed to verify execution semantics had to spawn a real shell, making the unit test suite dependent on Docker and slow. + +The two execution paths (`execute` and `executeParallel`) also duplicated their five phases verbatim, so any change had to be applied twice. + +## Decision + +Introduce a `ScriptRunner` function type as the seam between orchestration and OS process spawning: + +```go +type ScriptRunner func(command *config.Command, script string) error +``` + +`NewExecutor` accepts a `ScriptRunner` parameter. The production implementation, `NewShellRunner`, wraps the shell-exec logic (script preparation, stdio wiring, **Work dir** resolution, **Environment** layering). Tests inject a closure — no test struct or mock framework required. + +`Executor` no longer holds an `io.Writer`; output wiring belongs entirely to the runner. + +## Consequences + +- **Positive:** `Execute()` is now unit-testable without a real shell — inject a recording closure and assert on invocations and errors. +- **Positive:** All direct `os/exec` calls are confined to `shellRunner`; the orchestrator is pure Go logic. +- **Positive:** Enables a future dry-run mode by swapping in a no-op `ScriptRunner`. +- **Neutral:** Callers pass `executor.NewShellRunner(conf, out)` instead of `out` directly — a one-line change at each call site. +- **Neutral:** Debug-level-2 env logging moved into the runner; the orchestrator logs the script only. diff --git a/internal/cmd/help_test.go b/internal/cmd/help_test.go index 46fa5eae..e0c28b51 100644 --- a/internal/cmd/help_test.go +++ b/internal/cmd/help_test.go @@ -218,7 +218,7 @@ func TestErrorHandlerSplitsExecuteErrorCause(t *testing.T) { } conf := &configpkg.Config{Shell: "bash"} env.SetDebugLevel(0) - err := executor.NewExecutor(conf, nil).Execute(executor.NewExecutorCtx(context.Background(), command)) + err := executor.NewExecutor(conf, executor.NewShellRunner(conf, nil)).Execute(executor.NewExecutorCtx(context.Background(), command)) if err == nil { t.Fatal("expected executor error") } diff --git a/internal/cmd/subcommand.go b/internal/cmd/subcommand.go index b61d44f6..e2221f86 100644 --- a/internal/cmd/subcommand.go +++ b/internal/cmd/subcommand.go @@ -262,7 +262,7 @@ func newSubcommand(command *config.Command, conf *config.Config, showAll bool, o ctx := executor.NewExecutorCtx(cmd.Context(), command) - return executor.NewExecutor(conf, out).Execute(ctx) + return executor.NewExecutor(conf, executor.NewShellRunner(conf, out)).Execute(ctx) }, // we use docopt to parse flags on our own, so any flag is valid flag here FParseErrWhitelist: cobra.FParseErrWhitelist{UnknownFlags: true}, diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 69b7a76a..f674b59f 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -4,10 +4,7 @@ import ( "context" "errors" "fmt" - "io" - "os" "os/exec" - "strings" "github.com/lets-cli/lets/internal/checksum" "github.com/lets-cli/lets/internal/config/config" @@ -48,14 +45,14 @@ func (e *ExecuteError) ExitCode() int { type Executor struct { cfg *config.Config - out io.Writer + runner ScriptRunner initCalled bool } -func NewExecutor(cfg *config.Config, out io.Writer) *Executor { +func NewExecutor(cfg *config.Config, runner ScriptRunner) *Executor { return &Executor{ - cfg: cfg, - out: out, + cfg: cfg, + runner: runner, } } @@ -142,16 +139,10 @@ func (e *Executor) execute(ctx *Context) error { func (e *Executor) executeAfterScript(ctx *Context) { command := ctx.command - osCmd, err := e.newOsCommand(command, command.After) - if err != nil { - ctx.logger.Info("failed to run `after` script: %s", err) - return - } - - ctx.logger.Debug("executing 'after':\n cmd: %s\n env: %s", command.After, fmtEnv(osCmd.Env)) + ctx.logger.Debug("executing 'after':\n cmd: %s", command.After) - if ExecuteError := osCmd.Run(); ExecuteError != nil { - ctx.logger.Info("failed to run `after` script: %s", ExecuteError) + if err := e.runner(command, command.After); err != nil { + ctx.logger.Info("failed to run `after` script: %s", err) } } @@ -207,99 +198,6 @@ func (e *Executor) initCmd(ctx *Context) error { return nil } -func joinBeforeAndScript(before string, script string) string { - if before == "" { - return script - } - - before = strings.TrimSpace(before) - - return strings.Join([]string{before, script}, "\n") -} - -// Setup env for cmd. -func (e *Executor) setupEnv(osCmd *exec.Cmd, command *config.Command, shell string) error { - defaultEnv := e.cfg.CommandBuiltinEnv(command, shell, osCmd.Dir) - - checksumEnvMap := getChecksumEnvMap(command.ChecksumMap) - - var changedChecksumEnvMap map[string]string - if command.PersistChecksum { - changedChecksumEnvMap = getChangedChecksumEnvMap( - command.ChecksumMap, - command.GetPersistedChecksums(), - ) - } - - cmdEnv, err := command.GetEnv(*e.cfg, defaultEnv) - if err != nil { - return err - } - - envMaps := []map[string]string{ - defaultEnv, - e.cfg.GetEnv(), - cmdEnv, - command.Options, - command.CliOptions, - checksumEnvMap, - changedChecksumEnvMap, - } - - envList := os.Environ() - for _, envMap := range envMaps { - envList = append(envList, convertEnvMapToList(envMap)...) - } - - osCmd.Env = envList - - return nil -} - -// Prepare cmd to be executed: -// - set in/out -// - set dir -// - prepare environment -// -// NOTE: We intentionally do not passing ctx to exec.Command because we want to wait for process end. -// Passing ctx will change behavior of program drastically - it will kill process if context will be canceled. -func (e *Executor) newOsCommand(command *config.Command, cmdScript string) (*exec.Cmd, error) { - script := joinBeforeAndScript(e.cfg.Before, cmdScript) - - shell := e.cfg.Shell - if command.Shell != "" { - shell = command.Shell - } - - args := []string{"-c", script} - if len(command.Args) > 0 { - // for "--" see https://linux.die.net/man/1/bash - args = append(args, "--", strings.Join(command.Args, " ")) - } - - osCmd := exec.Command( - shell, - args..., - ) - - // setup std out and err - osCmd.Stdout = e.out - osCmd.Stderr = e.out - osCmd.Stdin = os.Stdin - - // set working directory for command - osCmd.Dir = e.cfg.WorkDir - if command.WorkDir != "" { - osCmd.Dir = command.WorkDir - } - - if err := e.setupEnv(osCmd, command, shell); err != nil { - return nil, err - } - - return osCmd, nil -} - // Run all commands from Depends in sequential order. func (e *Executor) executeDepends(ctx *Context) error { return ctx.command.Depends.Range(func(depName string, dep config.Dep) error { @@ -350,18 +248,11 @@ func (e *Executor) persistChecksum(ctx *Context) error { func (e *Executor) runCmd(ctx *Context, cmd *config.Cmd) error { command := ctx.command - osCmd, err := e.newOsCommand(command, cmd.Script) - if err != nil { - return err - } - if env.DebugLevel() == 1 { ctx.logger.Debug("executing script:\n%s", cmd.Script) - } else if env.DebugLevel() > 1 { - ctx.logger.Debug("executing:\nscript: %s\nenv: %s\n", cmd.Script, fmtEnv(osCmd.Env)) } - if err := osCmd.Run(); err != nil { + if err := e.runner(command, cmd.Script); err != nil { return &ExecuteError{err: fmt.Errorf("failed to run command '%s': %w", command.Name, err)} } diff --git a/internal/executor/runner.go b/internal/executor/runner.go new file mode 100644 index 00000000..e970e6f6 --- /dev/null +++ b/internal/executor/runner.go @@ -0,0 +1,110 @@ +package executor + +import ( + "io" + "os" + "os/exec" + "strings" + + "github.com/lets-cli/lets/internal/config/config" + "github.com/lets-cli/lets/internal/env" + log "github.com/sirupsen/logrus" +) + +// ScriptRunner executes a shell script in the context of a command. +type ScriptRunner func(command *config.Command, script string) error + +// NewShellRunner returns a ScriptRunner that spawns a real OS process. +func NewShellRunner(cfg *config.Config, out io.Writer) ScriptRunner { + r := &shellRunner{cfg: cfg, out: out} + return r.run +} + +type shellRunner struct { + cfg *config.Config + out io.Writer +} + +func (r *shellRunner) run(command *config.Command, cmdScript string) error { + script := joinBeforeAndScript(r.cfg.Before, cmdScript) + + shell := r.cfg.Shell + if command.Shell != "" { + shell = command.Shell + } + + args := []string{"-c", script} + if len(command.Args) > 0 { + args = append(args, "--", strings.Join(command.Args, " ")) + } + + // shell and script come from developer-authored lets.yaml, not user runtime input. + // User CLI args land in args as positional parameters ($1, $2, …) after "--", not as code. + osCmd := exec.Command(shell, args...) //nolint:gosec + osCmd.Stdout = r.out + osCmd.Stderr = r.out + osCmd.Stdin = os.Stdin + + osCmd.Dir = r.cfg.WorkDir + if command.WorkDir != "" { + osCmd.Dir = command.WorkDir + } + + if err := r.setupEnv(osCmd, command, shell); err != nil { + return err + } + + if env.DebugLevel() > 1 { + log.Debugf("executing:\n script: %s\n env: %s", cmdScript, fmtEnv(osCmd.Env)) + } + + return osCmd.Run() +} + +func (r *shellRunner) setupEnv(osCmd *exec.Cmd, command *config.Command, shell string) error { + defaultEnv := r.cfg.CommandBuiltinEnv(command, shell, osCmd.Dir) + + checksumEnvMap := getChecksumEnvMap(command.ChecksumMap) + + var changedChecksumEnvMap map[string]string + if command.PersistChecksum { + changedChecksumEnvMap = getChangedChecksumEnvMap( + command.ChecksumMap, + command.GetPersistedChecksums(), + ) + } + + cmdEnv, err := command.GetEnv(*r.cfg, defaultEnv) + if err != nil { + return err + } + + envMaps := []map[string]string{ + defaultEnv, + r.cfg.GetEnv(), + cmdEnv, + command.Options, + command.CliOptions, + checksumEnvMap, + changedChecksumEnvMap, + } + + envList := os.Environ() + for _, envMap := range envMaps { + envList = append(envList, convertEnvMapToList(envMap)...) + } + + osCmd.Env = envList + + return nil +} + +func joinBeforeAndScript(before string, script string) string { + if before == "" { + return script + } + + before = strings.TrimSpace(before) + + return strings.Join([]string{before, script}, "\n") +}