Introduce ScriptRunner seam into executor

[kindermax]

Summary

• Defines ScriptRunner as func(command *config.Command, script string) error — the seam between executor orchestration and OS process spawning
• Extracts ShellRunner (NewShellRunner(cfg, out)) as the production implementation, encapsulating shell selection, script prep, stdio wiring, work dir resolution, and env layering
• NewExecutor now accepts a ScriptRunner instead of io.Writer; runCmd and executeAfterScript both delegate to it, removing all direct os/exec calls from the orchestrator
• Adds docs/adr/0001-scriptrunner-seam.md documenting the decision

No observable behaviour changes. All existing go test ./... and Bats tests pass.

Closes #362

Test plan

• go test ./... — all packages pass
• lets test-bats (Docker) for CLI path changes

Summary by Sourcery

Introduce a ScriptRunner abstraction to decouple executor orchestration from OS process spawning and document the decision.

Enhancements:

• Refactor Executor to depend on a ScriptRunner function instead of an io.Writer, delegating script execution to the injected runner.
• Add a ShellRunner implementation that encapsulates shell selection, script preparation, environment setup, working directory resolution, and stdio wiring.
• Simplify executor debug logging to focus on script content while leaving env and process details to the runner implementation.

Documentation:

• Add an ADR documenting the introduction of the ScriptRunner seam and its implications for testing and future extensions.

[sourcery-ai]

Reviewer's Guide

Introduces a ScriptRunner abstraction as the seam between the executor orchestration and OS process spawning, moving shell/OS-specific logic into a new shellRunner implementation and updating executor construction and call sites accordingly, plus documenting the design via an ADR.

Sequence diagram for command execution via ScriptRunner

sequenceDiagram
    actor User
    participant CobraCmd as CobraCommand
    participant Executor
    participant ScriptRunner
    participant ShellProc as shellRunner

    User->>CobraCmd: Execute
    CobraCmd->>CobraCmd: newSubcommand()
    CobraCmd->>Executor: NewExecutor(conf, NewShellRunner(conf, out))

    User->>CobraCmd: run project command
    CobraCmd->>Executor: Execute(ctx)
    Executor->>Executor: runCmd(ctx, cmd)
    Executor->>ScriptRunner: runner(command, cmd.Script)
    ScriptRunner->>ShellProc: run(command, cmdScript)
    ShellProc->>ShellProc: exec.Command(...).Run()
    ShellProc-->>Executor: error or nil
    Executor-->>CobraCmd: error or nil
    CobraCmd-->>User: CLI exit code

Loading

File-Level Changes

Change	Details	Files
Executor now depends on a ScriptRunner seam instead of directly managing os/exec and I/O.	Replace Executor.out io.Writer field with runner ScriptRunner field and update constructor to accept a ScriptRunner instead of an io.Writer. Update executeAfterScript to delegate script execution to the ScriptRunner and simplify logging to omit env details. Update runCmd to call the ScriptRunner for script execution, simplify debug logging to only show the script, and remove all direct os/exec usage from executor.go.	internal/executor/executor.go
Introduce ShellRunner as the production ScriptRunner implementation encapsulating shell command construction, environment setup, stdio wiring, and working directory resolution.	Define ScriptRunner function type that executes a shell script for a given command. Add shellRunner struct holding Config and io.Writer and expose NewShellRunner constructor returning its run method as ScriptRunner. Move and adapt script joining, env setup, and exec.Command wiring from Executor into shellRunner.run and shellRunner.setupEnv, preserving previous behavior including checksum env handling, workdir resolution, and argument concatenation.	internal/executor/runner.go internal/executor/executor.go
Wire the new ScriptRunner seam into CLI entry points and tests.	Update subcommand creation to construct an Executor with executor.NewShellRunner(conf, out) instead of passing out directly. Update help_test to construct an Executor with a ShellRunner in place of the previous io.Writer-based constructor.	internal/cmd/subcommand.go internal/cmd/help_test.go
Document the ScriptRunner design decision.	Add ADR describing the motivation, decision, and consequences of introducing ScriptRunner and ShellRunner, including testability and separation of concerns benefits.	docs/adr/0001-scriptrunner-seam.md

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#362	Introduce a ScriptRunner abstraction type and a ShellRunner concrete implementation that wraps the existing OS process execution logic (shell selection, script prep including Before, args, env layering, work dir, stdio, and Run).	✅
#362	Refactor Executor to depend on an injected ScriptRunner: change NewExecutor to accept a ScriptRunner, and have runCmd and executeAfterScript delegate to it instead of constructing and running os/exec commands directly.	✅
#362	Update the production caller(s) to pass a ShellRunner instance into NewExecutor, preserving behavior so that all existing Go and Bats tests continue to pass without change.	✅

Possibly linked issues

• Introduce ScriptRunner seam into executor #362: PR fulfills all requested plumbing: ScriptRunner type, ShellRunner adapter, Executor injection, and runner-based command execution.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:

Security issues:

• Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)

General comments:

• The previous debug-level-2 behavior that logged the full command environment (fmtEnv(osCmd.Env)) for both main and after scripts has been dropped from runCmd/executeAfterScript and not reintroduced in shellRunner; if that output is still desired, consider adding environment logging in shellRunner.run (guarded by env.DebugLevel() > 1) to preserve the earlier behavior described in the ADR.
• Now that environment construction and execution are encapsulated in shellRunner, any future logging or behavior that depends on the derived env (e.g., for debugging or dry-run output) will require additional plumbing into the runner; you may want to plan a small interface or struct wrapper around ScriptRunner to make extending it (logging, metrics, tracing) easier than with a bare function type.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The previous debug-level-2 behavior that logged the full command environment (`fmtEnv(osCmd.Env)`) for both main and `after` scripts has been dropped from `runCmd`/`executeAfterScript` and not reintroduced in `shellRunner`; if that output is still desired, consider adding environment logging in `shellRunner.run` (guarded by `env.DebugLevel() > 1`) to preserve the earlier behavior described in the ADR.
- Now that environment construction and execution are encapsulated in `shellRunner`, any future logging or behavior that depends on the derived env (e.g., for debugging or dry-run output) will require additional plumbing into the runner; you may want to plan a small interface or struct wrapper around `ScriptRunner` to make extending it (logging, metrics, tracing) easier than with a bare function type.

## Individual Comments

### Comment 1
<location path="internal/executor/executor.go" line_range="251-252" />
<code_context>
-	}
-
-	if env.DebugLevel() == 1 {
+	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))
</code_context>
<issue_to_address>
**question (bug_risk):** The change to `env.DebugLevel() >= 1` removes the more detailed env logging for higher debug levels.

Previously `DebugLevel()==1` logged only the script, while `DebugLevel()>1` also logged the resolved environment. With `>= 1` and no separate `> 1` branch, there’s no way to see the expanded env at higher debug levels, which can hinder diagnosing env-related issues. If this change wasn’t intentional, consider restoring a separate `DebugLevel() > 1` path (possibly delegating env logging elsewhere).
</issue_to_address>

### Comment 2
<location path="internal/executor/runner.go" line_range="39" />
<code_context>
	osCmd := exec.Command(shell, args...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.

*Source: opengrep*
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}