Skip to content

Refactor: Consolidate ExCmd into CommandPort#5

Merged
dbernheisel merged 5 commits intotv-labs:mainfrom
lostbean:egomes/consolidate-excmd
Mar 27, 2026
Merged

Refactor: Consolidate ExCmd into CommandPort#5
dbernheisel merged 5 commits intotv-labs:mainfrom
lostbean:egomes/consolidate-excmd

Conversation

@lostbean
Copy link
Contributor

Note: This is PR 1 of 3, splitting #4 (sandbox) as requested by @dbernheisel. The stack is:

  1. This PR — Consolidate ExCmd into CommandPort
  2. Flexible command restrictions (targets this branch)
  3. Pluggable virtual filesystem (targets PR 2)

Motivation

External process spawning is spread across multiple modules — CommandPort, JobProcess, Coproc, Pipeline, and the command builtin each call ExCmd.Process.*, ExCmd.stream, or System.cmd directly. This makes it impossible to enforce a single policy (e.g. restricted mode) without patching every call site individually. It also makes the dependency on ExCmd implicit and hard to audit.

Changes

Consolidate ExCmd into CommandPort

CommandPort becomes the single interface for all user-facing OS process execution. It now exposes:

  • Low-level process API — thin delegates for os_pid, read, write, close_stdin, close_stdout, await_exit (used by JobProcess and Coproc which manage process lifecycles directly)
  • Streaming APIstream/3 wrapping ExCmd.stream (used by Pipeline)
  • System.cmd wrappersystem_cmd/4 (used by command builtin)
  • Restricted mode gatewaystart_link/3, stream/3, and system_cmd/4 accept a restricted boolean and return {:error, :restricted} when active
graph TD
    AST[AST.Command] --> CP[CommandPort]
    JP[JobProcess] --> CP
    CO[Coproc] --> CP
    PL[Pipeline] --> CP
    CM[Command builtin] --> CP
    CP -->|restricted?| ERR["{:error, :restricted}"]
    CP -->|allowed| EX[ExCmd / System.cmd]
Loading

Internal plumbing (System.cmd for kill, mkfifo, hostname, uname) is intentionally exempt.

After this change, ExCmd appears only in lib/bash/command_port.ex.

Bug fixes

  • Stderr forwarding from temp collectors — Pipeline execution and command substitution created temporary OutputCollector processes but discarded their stderr. Now stderr is forwarded to the session's stderr sink. This also fixes the 4 pre-existing golden test failures on main.
  • Relative path resolution in while loop redirectswhile read line; do ...; done < relative_file now resolves against session_state.working_dir instead of the BEAM's cwd.
  • :exec return handlingSessionCase.run_script/2 now handles the {:exec, result} return from Session.execute/2.

Test plan

  • All 2059 tests pass (0 failures, up from 4 pre-existing failures on main)
  • test/bash/command_port_test.exs — low-level API delegates, streaming, system_cmd, restricted mode errors, restricted?/1 helper
  • test/bash/stderr_forwarding_test.exs — stderr from $(), backticks, and pipeline temp collectors reaches the session
  • test/bash/control_flow_test.exs — while loop with relative path input redirect
  • grep -r "ExCmd\." lib/ --include="*.ex" returns only command_port.ex

Forward stderr from temporary OutputCollectors in pipeline execution
and command substitution to the session's stderr sink instead of
discarding it. Resolve relative file paths in while loop input
redirects against the session's working directory. Handle :exec
return value in SessionCase test helper.
Expand CommandPort to be the single interface for all user-facing OS
process execution. Add thin delegates for ExCmd.Process lifecycle
operations (os_pid, read, write, close_stdin, close_stdout, await_exit)
and gateway functions with restricted-mode enforcement (start_link,
stream, system_cmd). Internal helpers now call public delegates instead
of ExCmd.Process directly.
Replace direct ExCmd.Process and System.cmd calls in JobProcess,
Coproc, Pipeline, and Command builtin with CommandPort delegates.
CommandPort is now the single gateway for all user-facing OS process
execution. Add restricted-mode guard to external_command? in Pipeline
to prevent streaming bypass.
Copy link
Collaborator

@dbernheisel dbernheisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great -- Just one suggestion to move restricted? to Session because that feels like a session concern that the CommandPort needs to adhere to. Then I'm ready to merge.

Per review feedback, restricted mode is a session concern — not a
process-spawning concern. Moves the helper and updates all callers.
@dbernheisel dbernheisel merged commit c989f10 into tv-labs:main Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants