# p0147e: PipelineExecutor decomposition

- [Architecture] Decompose PipelineExecutor by composition into three sub-services: `IPipelineStepRunner` (one step in/out: factory → executor → progress → exception envelope), `IPipelineErrorHandler` (failure policy: HTML ticket comment + WIP-persist guard + lifecycle marking), `IPipelineSandboxCoordinator` (lazy ISandbox boot + dispose). Three is the right shape — progress reporting and lifecycle marking are one-line pass-throughs that don't need their own service.
- [Architecture] Keep dynamic-command-insertion in the orchestrator. Mutating the LinkedList mid-loop is part of the iteration logic itself; extracting it would either need callback hell or a custom iterator.
- [Architecture] `IPipelineSandboxCoordinator` is `IAsyncDisposable` and registered transient. Owns mutable per-pipeline state (the cached sandbox); singleton would share one ISandbox across overlapping runs.
- [Architecture] Orchestrator resolves the sandbox coordinator via `IServiceProvider.GetRequiredService` inside `ExecuteAsync` rather than via constructor injection, so each pipeline call gets a fresh coordinator regardless of how the executor itself is registered.
- [Implementation] Parallel-class migration — rename the existing 503-line executor to `PipelineExecutorLegacy`, build the new shape alongside, gate selection by `PIPELINE_EXECUTOR_USE_LEGACY` env var (defaults to new). The full pipeline test pack is parametrised across both shapes via `[Theory]` so any observable divergence shows up as a failing assertion.
- [Implementation] `PipelineExecutor` (orchestrator) is 156 lines total: 28-line `ExecuteAsync` entry + 53-line `RunLoopAsync` + small helpers. Each sub-service stays in the 120-250 line range. Total of all four files (652 lines) is up vs the 503-line monolith because each file's docblock + namespace block + per-service test seams add fixed overhead — the value is per-file SRP, not total line count.
- [Implementation] `PipelineExecutor` exposes an `internal static PeelBatch` shim that delegates to `PipelineStepRunner.PeelBatchInternal` so the existing `PipelineExecutorBatchingTests` keep compiling without touching call sites. The shim is internal + `InternalsVisibleTo("AgentSmith.Tests")`.
- [Implementation] `IPipelineStepRunner.RunBatchAsync` returns failure WITHOUT posting the HTML status comment — that side effect moves to `IPipelineErrorHandler.HandleStepFailureAsync`. The orchestrator hands a failed step result to the error handler, which posts the comment + persists the WIP branch + marks lifecycle failed in that order.
- [TradeOff] Migration via parallel-class instead of in-place refactor (rejected: 503-line files are exactly where a corner case can disappear in a giant diff). The parallel-class approach doubles the file count for one release cycle, but every test runs both shapes — that's the migration safety net.
- [TradeOff] The new shape adds an `IServiceProvider` dependency to the executor. Service-locator pattern is generally a smell, but the alternative (factory function delegate injected by DI) was no simpler and the call site is one line. Acceptable for this single use case.
