Conversation
`write_stdin` was line oriented: it split the payload into lines and appended a
newline to any line missing one. That is right for interactive input, but it
corrupts file contents - a binary buffer raised `TypeError` (comparing `bytes`
to `str` in `line.endswith("\n")`) and text gained a spurious trailing newline.
`_stdin` now also accepts `bytes` and binary file objects, which are written
through verbatim. Binary streams are copied with `shutil.copyfileobj` so large
payloads are chunked rather than read into memory. Text payloads keep their
existing line by line behaviour, so `str`, `list[str]` and `StringIO` are
unaffected.
This makes it possible to pipe a file into a remote command, e.g.
`cat > dest` fed from a local file, without staging a temporary copy on the
target - useful for connectors that reach a host through intermediate hops.
Note that stdin is fully written before any output is read, so a command
emitting more than a pipe buffer of output while consuming a large payload will
deadlock. That is pre-existing behaviour, now documented on `write_stdin`.
Counterpart to the binary `_stdin` support: `_stdout` takes a writable binary buffer and receives the command's raw stdout, chunked through `shutil.copyfileobj`, instead of having it decoded into `CommandOutput` lines. Command output is otherwise read line by line and decoded as UTF-8, which is right for facts and logging but cannot carry arbitrary bytes. A sink makes it possible to read a remote file straight into a local one, with no temporary copy on either side. The argument is opt-in and strictly additive: when it is absent nothing changes. When set, only stdout is diverted - stderr is still captured as text, so errors stay reportable and the sudo password prompt detection in `execute_command_with_sudo_retry` keeps working, as sudo writes those to stderr. Diverted output is not printed either, having no text form. `_stdout` is listed in `extract_control_arguments` so that wrapping connectors such as `@docker` and `@chroot` forward it to the connector actually holding the pipe.
…ct isolation
Blocking:
- Rewind a stream `_stdin` and reset a `_stdout` sink before a retry, in
`execute_command_with_sudo_retry` and in the operation `_retries` loop. A
file-object payload is consumed by the first attempt, so the retry used to
send an empty stdin - truncating the target of a `cat > dest` upload, eg
files.put(..., _sudo=True) through a chain - and to append its output to the
first attempt's. A non-seekable stream now raises rather than silently
sending nothing.
- Raise a failed sink write instead of reporting success: gevent.wait returns
the greenlets that finished, not the ones that succeeded, so an OSError
(disk full, closed file) left the command green with a truncated sink, or was
misreported as a TimeoutError. The peer reader is killed on failure,
otherwise the undrained pipe hangs the call.
- Detect binary streams duck-typed instead of RawIOBase/BufferedIOBase:
tempfile's wrappers, paramiko's channel files and gevent's file objects are
file-like but not io subclasses, and fell through to the line handler with
"TypeError: endswith first arg must be bytes". Text streams are recognised by
`TextIOBase` as well as `encoding`, as `StringIO` has no `encoding`.
- Exclude `_stdin`/`_stdout` from fact commands. Facts inherit the globals of
the operation that triggered them, so a sink swallowed the fact's stdout and
the fact returned its default - files.line(..., _stdout=f) appended the line
twice - and a stream payload was consumed by the fact before the operation's
own command ran.
- Accept `bytearray`/`memoryview`, and treat `b""` as a real payload: the
truthy checks dropped empty binary input, eg truncating a file.
- Reject `_stdout` together with `_get_pty`: a pseudoTTY merges stderr into
stdout, so the sink would collect prompts and errors instead of the command's
output, and sudo prompt detection would stop seeing them.
Minor:
- Accept duck-typed binary buffers as `_stdout`; the runtime check rejected
tempfile's wrappers while `_stdin` accepted them.
- Annotate `write_stdin` fully, close its buffer once, and drop the
cast("IO[bytes]", ...) that papered over the buffer annotations: the channel
files connectors pass are not `IO[bytes]`.
Docs:
- List `_stdout` with the other connector control parameters that must be
filtered before `make_unix_command_for_host`.
…t ones
`write_stdin` wrapped anything that was not a list or a tuple - including the
iterables `_stdin` accepts - so a generator of lines had `endswith` called on
the generator itself, and every other non-text iterable failed the same way
with "no attribute 'endswith'". The argument type check cannot see inside an
arbitrary iterable, so a buffer such as `array('B')` passes it and reached the
same line handling.
Iterate any iterable as lines, and raise a clear error when an element is not
text. Bytes, bytes-like values, binary file objects and file-like buffers such
as `mmap` never reach the line handling and are unaffected.
maisim
marked this pull request as draft
September 25, 2026 11:48
wowi42
added
new feature
connectors
Contributor
Author
|
I use it since a while (via the chain connector) but just did a refactor, be careful if you want to play with. Feedback are welcome |
Both were spelled out as unions wherever they were passed around, and `execute_command_with_sudo_retry` took `Any | None`, which says nothing about what it accepts. Name them next to `EnvValue`: `StdinPayload` for what `_stdin` takes, `StdoutSink` for what `_stdout` takes. `arguments_typed.py` imports them rather than re-declaring them the way `EnvValue` is duplicated - one definition cannot drift from its copy, and the sync linter compares usages only, so a drift would go unnoticed. The retry helpers and `write_stdin` keep loose annotations deliberately: they probe the payload at runtime instead of assuming a shape. Generated documentation is unchanged: `get_type_hints` resolves an alias to the union underneath it, so the arguments page still lists the full type.
…n sink error A retry rewound a seekable `_stdin` to 0 rather than to where the first attempt started, so a stream positioned past a header re-sent the header. The initial position is now captured before the first attempt, in both the operation retry loop and the sudo password retry. When the `_stdout` sink failed, `run_local_process` raised without terminating or reaping the child, which could stay blocked on the undrained pipe. The child is now terminated, waited for, and its pipes closed on any error.
`StringIO` passed the class based runtime check (it is an `Iterable`) and failed later on the first `write(bytes)` with a `TypeError` from `copyfileobj`. A handler on the argument now refuses `TextIOBase` instances with an `ArgumentTypeError` naming `_stdout`.
The non-seekable check lived in `rewind_stdin_for_retry`, so it fired on the retry: the first attempt had already consumed the stream (and, for `cat > dest`, truncated the destination) before the operation failed. `check_stdin_is_rewindable` now runs before the retry loop, and the seekable probe is shared by the three retry helpers.
The whole `_stdin` payload was written before any output was read, so a command that emits more than a pipe buffer of output while consuming a large payload deadlocked - each side blocked on a pipe the other was not draining. `read_output_buffers` now takes the stdin buffer and payload and writes them from a third greenlet alongside the two readers; a failed write surfaces like a failed sink write, ahead of the exit status or a timeout. Pinned by a real-process test that floods stderr before reading a 1 MiB stdin.
`get_file_io` converted `StringIO` for binary *reads* only, so a connector streaming a command's raw stdout into a text destination - `files.get` with a `StringIO` - failed on the first `write(bytes)`. Opened with `mode="wb"`, a `StringIO` is now stood in for by a `BytesIO` and receives the decoded bytes once the block completes; a failed write leaves it untouched.
Three of the tests added with the binary `_stdin` payloads only ran where a POSIX shell and POSIX pipes exist, and the Windows leg failed on them: on `cmd.exe`, `head -c 1048576 /dev/zero | tr '\0' e >&2; wc -c` and `exec yes` are not commands, the child exits at once, and writing the payload into its closed stdin raises BrokenPipeError instead of reaching the code under test. They now drive this interpreter rather than a shell - the command's source in a temporary file, so it carries no shell metacharacters and quotes the same way under `sh` and `cmd.exe` - and the non-seekable stream is built in Python, so no pipe is involved. What is under test is unchanged, and it is exercised on every platform the connector supports.
maisim
force-pushed
the
feature/binary-stdin
branch
from
October 4, 2026 07:25
0ea5575 to
e2f577f
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The need is to copy files via the "chain" connector without writing to intermediate hosts: this avoids issues with available disk space and eliminates unnecessary disk writes.
Adds binary payload support to the two connector I/O control arguments:
_stdinacceptsbytes/bytearray/memoryviewand binary file objects, and the new_stdouttakes a binarybuffer that receives the command's raw stdout. Both are opt-in and additive.
Why. Command output was always decoded as UTF-8 into
CommandOutputlines, and_stdinonlycarried text. A connector whose only channel is a shell command therefore cannot move a file byte
for byte:
@chrootand@dockerstage a local temporary file and read the whole payload intomemory before copying it, and a connector with no
cp/docker cpequivalent has no option at allshort of base64 in a command line. Streaming both directions fixes that, and it is what
@chainbuilds its file transfers on: no temporary copy per layer, no buffering, raw bytes end to end.
It is directly usable too, without waiting for a connector to adopt it:
stderr stays captured as text, so errors remain reportable and the sudo password prompt detection
in
execute_command_with_sudo_retrykeeps working (sudo writes those to stderr). Diverted outputis not printed, having no text form. Both arguments are listed in
extract_control_arguments, sowrapping connectors forward them to the connector that holds the pipe.
A review pass over the new code paths found and fixed:
_retries) re-ran the command with an exhausted_stdinanda sink that was never rewound:
cat > desttruncated the target to zero bytes and reportedsuccess
RawIOBase/BufferedIOBase, missing tempfile's wrappers, paramiko'schannel files and gevent's file objects
_stdin/_stdoutfrom the operation that triggered it, so a sink swallowed thefact's output and the fact silently returned its default
bytearray/memoryviewpayloads were rejected, empty bytes were dropped, and an_stdiniterable of lines was wrapped in a list, so generators never worked
_stdoutwith_get_ptymerged stderr into the sink, hiding the sudo prompt_stdoutis annotatedIO[bytes] | Iterable[bytes]because the runtime type check on globalarguments is class based, so a plain
IO[bytes]rejects duck typed binary buffers such astempfile's wrappers;
_stdincarries the same widening. Tests intests/test_connectors/test_local.py,test_connectors/test_ssh.pyandtests/test_api/test_api_facts.pycover each fix, and the retry one is pinned by a test that failswhen the rewind is removed.
AI usage disclosure
Written with DeepSeek 4.1 Flash. AI assistance covered: drafting the implementation, the
tests and the documentation; running the test suite and the linters; and drafting this
description. I reviewed, edited and tested every change, and I can explain any part of it without
the tool.
Checklist
3.x)docs/api/connectors.md;_stdoutalso appears in the generated globalarguments page
scripts/dev-test.shscripts/dev-lint.sh