ADR 0001: Faithful subprocess output capture

Status

Proposed. 2026-06-20.

Context

The legacy command runner libvcs._internal.run.run — used by every Git, Hg, and Svn command class via .run() — does not return what the underlying VCS actually printed. After the process exits it splits captured stdout into lines, calls bytes.strip() on each line, drops any line that is empty after stripping, and rejoins with \n and no trailing newline. stderr is treated the same way and then rejoined with no separator at all.

That post-processing corrupts any output where whitespace is significant:

  • Leading indentation is removed from every line.

  • Blank lines (including a unified-diff blank context line, which is a single space) are dropped.

  • The trailing newline is removed.

  • Multi-line error messages are concatenated word-against-word, because stderr lines are rejoined with an empty separator.

The user-visible consequence: a diff captured through .run(["diff"]) cannot be re-applied. git apply requires every patch line — including the final one — to be newline-terminated, so a stripped diff is rejected as corrupt patch. The same corruption silently damages git show, git cat-file blob (file contents), git format-patch, and any --format output that carries indentation or blank lines. Single-token reads such as rev-parse HEAD are unaffected, which is why the defect went unnoticed.

This is not a regression. The line-cleanup dates to the project’s original progress-callback code, where it was meant to tidy human-readable progress lines, not to capture structured output. The runner’s own module docstring already states that it “will be deprecated by libvcs._internal.subprocess”.

libvcs._internal.subprocess.SubprocessCommand already exists as a thin, typed wrapper that returns a real subprocess.CompletedProcess with separate, untouched stdout and stderr. It is bytes-first with opt-in text decoding, and it is currently wired into nothing.

Decision

Subprocess output is captured pristine. Any trimming or decoding is a deliberate transformation applied at the call site, never inside the capture path. This is implemented in two phases so the stable .run() -> str contract is never broken.

Phase 1 — stop corrupting in the capture path

  • Capture stdout and stderr as raw bytes and decode once, without per-line stripping or blank-line dropping. Interior whitespace, blank lines, and stream structure are preserved exactly.

  • .run() returns the captured output verbatim by default, including the trailing newline, so whitespace-significant output (diffs destined for git apply, blob contents) round-trips byte-for-byte.

  • Add a trim=True opt-in that applies a single whole-output rstrip() for the convenient “bare value” reads where a trailing newline is just noise (e.g. rev-parse HEAD). Trimming is a deliberate caller choice, never the capture default.

  • Preserve stderr’s line structure for error messages, and decode it tolerantly: the UTF-8 fallback uses errors="backslashreplace", so an undecodable byte surfaces as an escape sequence in libvcs.exc.CommandError.output instead of raising UnicodeDecodeError.

Phase 2 — pristine structured backend

  • Route the cmd/* classes through libvcs._internal.subprocess.SubprocessCommand, which returns a subprocess.CompletedProcess (bytes-first, separate stdout/stderr/returncode).

  • Expose a structured accessor that returns the CompletedProcess for callers that want streams, exit code, and exact bytes. Keep .run() -> str (verbatim by default, trim=True opt-in) as the convenience facade over it.

  • Retire libvcs._internal.run.console_to_str in favor of subprocess’s own decoding with errors="backslashreplace".

  • Let the legacy libvcs._internal.run.run deprecate, as its docstring already anticipates.

Alternatives considered

Each candidate was measured against the test suite and a probe checking whether a captured diff survives git apply and whether cat-file blob is byte-identical.

Approach

diff applies

blob identical

rev-parse bare

errors intact

tests failing

Per-line strip + drop-blanks (original)

no

no

yes

no

baseline

Whole-output rstrip default

no

no

yes

yes

1

Verbatim default + trim=True opt-in (chosen, Phase 1)

yes

yes

opt-in

yes

doctests only

Structured CompletedProcess (Phase 2)

yes

yes

edge decides

yes

0 (via facade)

The decisive measurement: flipping the default to verbatim broke only doctests in cmd/git.py, cmd/hg.py, and cmd/svn.py — example output that gained a trailing newline. No functional test, sync-layer call, or downstream consumer broke, because those already strip where they need a bare value (vcspull, like the sync layer, trims defensively). A global trailing trim, by contrast, cannot produce an applyable patch: the patch’s required final newline is exactly what it strips. So verbatim becomes the default — fixing the original git apply failure for the plain .run(["diff"]) call — and trimming is an explicit trim=True opt-in for bare-value reads.

Every affected doctest was updated to show the real verbatim output. The Svn.blame doctest is a notable case: its original expected value had encoded the old bug (column-padding spaces already stripped), so it now reflects the true, faithful output.

Consequences

Positive

  • Captured diffs and patches re-apply by default; blob reads are byte-identical; error messages keep their line structure.

  • The default now returns output with its trailing newline. Callers that want a bare value pass trim=True; existing consumers are unaffected because the sync layer and vcspull already strip defensively before comparing.

  • The stderr concatenation defect is removed: error lines keep their separators. (Phase 2’s structured backend additionally avoids the pipe-buffer deadlock the legacy poll loop can hit when a child floods stdout.)

Tradeoffs

  • Callers that relied on the implicit trailing-newline trim must now pass trim=True (or strip themselves) for bare-value reads.

  • Phase 2 introduces a second return shape (CompletedProcess) alongside the string facade, and migrates the command classes onto a new backend.

Risks

  • Behavior drift between the .run() facade and the structured result. Mitigation: the facade decodes the structured result’s stdout (verbatim by default; trim=True applies rstrip), not a separate code path.

  • Encoding surprises on non-UTF-8 output. Mitigation: strict decode for data with a documented backslashreplace fallback for diagnostics, following the channel-split used by the tools surveyed below.

Prior art

The decision follows the convergent practice of mature VCS and subprocess-wrapping tools, none of which trim inside the capture path:

  • pip added a per-call mode (stdout_only) that returns VCS output verbatim, and replaced its console_to_str helper with errors="backslashreplace".

  • uv captures into a raw Output { stdout, stderr } and applies trim_end() at each call site for scalar reads.

  • mise relies on whole-output trailing trim for scalars, decodes strictly for data and lossily for stderr.

  • Mercurial, gitoxide, and Jujutsu are bytes-first and keep output verbatim; gitoxide treats newline-stripping as a named, opt-in view, and tracks the missing-final-newline case explicitly.

  • git itself confirms the constraint: its patch parser requires each line, including the last, to be newline-terminated.

The lesson shared by all of them: capture pristine, keep streams separate, and make trimming and decoding explicit edge transformations. SubprocessCommand already embodies that shape inside libvcs.