RFC: Introduce ports-and-adapters seam for handlers (IExecutor, IRenderer, IGitRepo) with a thin Run.* sugar layer

[stevehansen]

Summary

Refactor the ~80 command handlers in SafeCommands so safety claims become testable and cross-handler duplication of git state probes goes away. Concretely: introduce three ports (IExecutor, IRenderer, IGitRepo), wire them via constructor injection through a Ports record, and layer a thin Run.* sugar facade on top so the 50% pure-passthrough case stays a one-liner. No new dependencies; no DI container; no middleware.

This RFC is the result of an architectural exploration that compared four alternative designs (minimal interface, max-flexibility framework, common-case sugar, ports-and-adapters). The chosen design is a hybrid: ports-and-adapters as the foundation, with common-case sugar layered on top.

Motivation

Two architectural pains, recurring across the codebase:

Pain 1 — Command execution boilerplate. Roughly 80 handlers repeat the same six-line ritual:

var (code, output, error) = ProcessRunner.Run("tool", args);
if (json) OutputFormatter.WriteJson(new { exitCode = code, output, error });
else { OutputFormatter.WritePassthrough(output); OutputFormatter.WritePassthroughError(error); }
return code;

Identical blocks live in BunCommands.cs:34-45, DotnetCommands.cs:35-46, DockerCommands.cs:41-52. Refactoring the JSON shape today means touching ~80 sites. ProcessRunner.RunPassthrough (Infrastructure/ProcessRunner.cs:48-66) is dead code.

Pain 2 — Safety validation scattered. The same shape ("match args/state against a deny rule, emit structured Blocked(reason, suggestion)") is reimplemented across seven files in three different idioms:

• GitCommands.cs:329 — exact-match HashSet (PushBlockedFlags)
• DbCommands.cs:75-88 — BlockIfDestructive helper, lowercases first
• DbCommands.cs:199-211 (RunArtisanMigrate) — inline arg.Contains("fresh"|"reset"|"rollback"|"wipe")
• FileCommands.cs:36-55 — ValidatePath for path traversal
• GitCommands.cs:53-84 — IsWorkingTreeClean, IsGitRepo, RequireCleanTree
• GitCommands.cs:280-298 — RunCommitAmend runs three rev-parse calls to check whether HEAD is pushed
• FileCommands.cs:439-447 — RunDeleteTracked runs two more git probes (ls-files, diff)
• ProxyCommands.cs:177-189 — inline curl POST/PUT block
• NpmCommands, BunCommands, PnpmCommands — copy-pasted AllowedScripts HashSets

The deeper structural problem behind both pains: safety claims are unverifiable. The codebase has zero tests, and handlers cannot be tested without spawning real processes — they call static ProcessRunner.Run directly, with no seam. Today there is no way to assert "safe git push --force is blocked" without invoking the full CLI as a subprocess. STRIDE.md tracks this implicitly through R1 (no audit trail, score 9), but the test gap is the underlying enabler.

Proposed design

Three ports

// Infrastructure/Ports/IExecutor.cs
public interface IExecutor
{
    ExecResult Run(string tool, IReadOnlyList<string> args, ExecOptions? opts = null);
}
public readonly record struct ExecResult(int ExitCode, string StdOut, string StdErr);
public readonly record struct ExecOptions(string? Cwd = null,
    IReadOnlyDictionary<string, string>? Env = null);

// Infrastructure/Ports/IRenderer.cs
public interface IRenderer
{
    void Passthrough(string stdout, string stderr);
    void Info(string message);
    void Blocked(string reason, string? suggestion);
    void Json(object payload);
    bool JsonMode { get; }
}

// Infrastructure/Ports/IGitRepo.cs
public interface IGitRepo
{
    bool IsRepo();
    bool IsWorkingTreeClean();
    bool IsFileTracked(string relativePath);
    bool HasUnstagedChanges(string relativePath);
    HeadStatus GetHeadStatus();
}
public readonly record struct HeadStatus(string? Branch, string? Upstream, bool IsPushed);

IGitRepo is the port that earns its keep — it deduplicates git state probes that today live in both GitCommands and FileCommands. The three rev-parse calls in RunCommitAmend and the ls-files/diff calls in RunDeleteTracked move behind this port.

Adapters

• ProcessExecutor — instance wrapper over the existing ProcessRunner.Run mechanics. Behavioural no-op.
• ConsoleRenderer — wraps OutputFormatter. Constructed with the json flag.
• GitRepoAdapter — takes IExecutor in its constructor; answers structured questions via git invocations.
• FakeExecutor — recording fake; predicate-keyed canned results; tests assert on Calls.
• FakeRenderer — accumulates rendered output and BlockedReasons.
• FakeGitRepo — fluent builder: new FakeGitRepo().AsRepo().WithCleanTree().WithPushedHead("main", "origin/main").

Handler shape

Group classes take a Ports record by constructor. The registry stores a factory rather than a static delegate.

public sealed record Ports(IExecutor Exec, IRenderer Render, IGitRepo Git);
public delegate int CommandHandler(string[] args);

// Registration:
registry.Add("git", "push", "Push commits", Safety.SafeWrite,
    ports => args => new GitCommands(ports).RunPush(args));

Ports is a record so adding a fourth port (IAuditLog, IClock) doesn't ripple through 80 signatures.

Sugar layer for the 50% case

public static class Run
{
    public static int Tool(Ports p, string tool, string[] args, Policy? policy = null)
    {
        if (policy?.Evaluate(args) is Block b) {
            p.Render.Blocked(b.Reason, b.Suggestion);
            return 1;
        }
        var r = p.Exec.Run(tool, args);
        return p.Render.Result(r);  // handles json/passthrough
    }
    public static int Bun(Ports p, string sub, string[] args)
        => Tool(p, "bun", [sub, ..args]);
    public static int Git(Ports p, string sub, string[] args, Policy? pol = null)
        => Tool(p, "git", [sub, ..args], pol);
    // ...dotnet, docker, npm, pnpm
}

Policy

Policy is a pure record with fluent builders (DenyFlags, AllowOnlyScripts, SandboxPath, Require). Its Evaluate(args) method is unit-testable in isolation with no infrastructure.

Verbosity gradient (illustrative)

// Pure passthrough — 1 line
public int RunBun(string[] args) => Run.Bun(_ports, "run", args);

// Flag-block + run — 1 line
public int RunPush(string[] args) =>
    Run.Git(_ports, "push", args, Policy.Default.DenyFlags("--force", "-f"));

// Multi-step probe via IGitRepo — drops to raw ports
public int RunCommitAmend(string[] args)
{
    if (!_git.IsRepo()) { _render.Blocked("Not a git repository", null); return 1; }
    var head = _git.GetHeadStatus();
    if (head.IsPushed) {
        _render.Blocked($"HEAD is pushed to {head.Upstream}; amending would rewrite published history.",
                        "Make a new commit instead.");
        return 1;
    }
    return Run.Git(_ports, "commit", ["--amend", ..args]);
}

Composition root

static int Main(string[] argv)
{
    bool json = argv.Contains("--json");
    var exec = new ProcessExecutor();
    var render = new ConsoleRenderer(json);
    var git = new GitRepoAdapter(exec);
    var ports = new Ports(exec, render, git);

    var registry = CommandRegistry.Build();
    var (group, cmd, rest) = Dispatch.Parse(argv);
    var handler = registry.Resolve(group, cmd)?.Factory(ports);
    return handler?.Invoke(rest) ?? Help.Show(group);
}

Designs considered and rejected

Four parallel designs were drafted with different design constraints; only the hybrid above was selected.

• Minimal interface — single Safe.Execute(SafeSpec) entry point with a fluent record. Rejected: SafeSpec becomes a kitchen sink of five orthogonal concerns; multi-step probes are positionally addressed (ctx.Probes[0], [1], [2]); Generate group requires a synthetic Tool = "internal:generate" adapter, leaving two idioms in the codebase.
• Maximum flexibility framework — ICommandHandler + IPolicy + IProbe + IExecutor + IOutputParser + IRenderer + ICommandMiddleware + DI host. Rejected: seven new concepts to learn; assumes Microsoft.Extensions.DependencyInjection (violates the "no new deps" constraint); plugin-assembly loading is a security smell for a tool whose pitch is safety. Audit logging (its big win) can be added later as a fourth port without restructuring.
• Common-case sugar only — static Run.Tool facade with settable Run.Executor. Rejected as a standalone design: settable static makes parallel tests unsafe; Policy grows tool-specific methods on a generic type. Adopted as the sugar layer on top of the ports design.

Test impact

Currently: zero tests. After this refactor:

[Fact]
public void GitPush_ForceFlag_IsBlocked_WithoutSpawningProcess()
{
    var exec = new FakeExecutor().Default(0, "", "");
    var render = new FakeRenderer();
    var git = new FakeGitRepo();
    var sut = new GitCommands(new Ports(exec, render, git));

    var rc = sut.RunPush(new[] { "--force", "origin", "main" });

    Assert.Equal(1, rc);
    Assert.Empty(exec.Calls);                                // never spawned git
    Assert.Contains("--force is blocked", render.BlockedReasons[0]);
}

[Fact]
public void CommitAmend_Blocked_WhenHeadIsPushed()
{
    var git = new FakeGitRepo().AsRepo().WithPushedHead("main", "origin/main");
    var sut = new GitCommands(new Ports(new FakeExecutor(), new FakeRenderer(), git));
    Assert.Equal(1, sut.RunCommitAmend(new[] { "-m", "tweak" }));
}

The previously-impossible assertion — Assert.Empty(exec.Calls) after a blocked policy — becomes a one-liner. Estimated test count after the refactor: ~50–80 policy unit tests + ~15 IGitRepo-driven handler tests, all sub-millisecond.

Migration plan

Phased, mergeable in slices. Each phase ships independently and leaves the codebase in a working state.

1. Phase 1 — Add ports and adapters. Land IExecutor, IRenderer, IGitRepo plus ProcessExecutor, ConsoleRenderer, GitRepoAdapter. No handler changes yet. Wire Ports in Program.cs but keep static ProcessRunner.Run / OutputFormatter.* as a transitional shim. (~150 LOC added, 0 changed.)
2. Phase 2 — Add Policy + Run sugar. Land Policy record, Run.Tool / Run.Bun / Run.Git / etc. Add a small test project. Migrate one command group end-to-end (suggest BunCommands — small, all passthroughs) as a worked example.
3. Phase 3 — Migrate flag-block groups. Dotnet, Docker, Npm, Pnpm, Db. Collapse the duplicated AllowedScripts HashSets into a single shared one. Add policy unit tests as each migrates.
4. Phase 4 — Migrate GitCommands and FileCommands. This is where IGitRepo pays off: the three rev-parse calls in RunCommitAmend and the two probes in RunDeleteTracked move behind GetHeadStatus / IsFileTracked / HasUnstagedChanges. Add tests using FakeGitRepo.
5. Phase 5 — Migrate ProxyCommands and enforce the allowlist. The flag allowlist in ProxyCommands.cs:12-144 is currently documentation only — never validated against incoming args. Migration adds real enforcement.
6. Phase 6 — Remove the transitional shim. Delete ProcessRunner.RunPassthrough (dead code). Static ProcessRunner.Run callers are gone; class can become internal or be deleted.

Out of scope

• Audit logging (STRIDE R1). Adding a fourth IAuditLog port is mechanical once the seam exists; tracked separately under Add audit logging for command execution #1.
• Streaming process output (e.g., live terraform plan rendering). IExecutor returns captured output; streaming would be a future second method.
• DI container, middleware, plug-in assemblies. Explicitly rejected during design.
• Async port surface. SafeCommands is a synchronous CLI; async is YAGNI.

Risks

• IGitRepo could grow into a god-port. Counter: every method must be answerable by one git invocation, and policy stays out. New questions = new methods, not parameters.
• Stack traces gain a frame. Acceptable; the test-ability win dominates.
• Spectre.Console's static AnsiConsole is a back door around IRenderer. A code-review convention (or a Roslyn analyzer) is needed to keep new code from regressing to direct console writes.
• Migration across 80 handlers is real work. Mitigated by phasing; each phase is independently shippable.

Update STRIDE.md after merging

Per CLAUDE.md, update STRIDE.md once this lands: the seam introduced here is a prerequisite for resolving R1 (no audit trail) and improves verifiability of the existing T-class mitigations (flag blocks, path sandboxing).