Skip to content

Make tmux command execution pluggable, default path unchanged - #739

Open
tony wants to merge 16 commits into
masterfrom
engine-seam-minimal
Open

Make tmux command execution pluggable, default path unchanged#739
tony wants to merge 16 commits into
masterfrom
engine-seam-minimal

Conversation

@tony

@tony tony commented Aug 13, 2026

Copy link
Copy Markdown
Member

Bottom of a two-PR stack. #742 (typed operations and engines) is based
on this branch and lands after it.

Summary

  • Add a TmuxEngine seam: every tmux command libtmux runs — Server.cmd(), the listing queries behind Server.sessions, and Server.raise_if_dead() — goes through an engine object that takes a rendered argv and returns a structured result.
  • Add Server(engine=…) as the injection point. TmuxEngine is a typing.Protocol, so any object with run() and run_batch() qualifies; there is no base class to inherit and no dependency on libtmux's class hierarchy.
  • Keep the default path byte-for-byte: SubprocessEngine forks the tmux binary as before, cmd() still returns tmux_cmd, and arguments still reach tmux unchanged.
  • Consolidate binary lookup and the -L/-S/-f/-2/-8 flags into one ServerConnection. Three copies previously disagreed about which flags to emit, which is why config_file= and colors= were honored on some commands and not others.
  • Change tmux_cmd.process from a plain attribute to a read-only property. Reading it under the default engine is unchanged; it raises when the engine forked no process.
  • Guard the injection footgun: an engine that names no tmux server of its own adopts the server's connection, so injecting one into a socket-scoped Server cannot silently dispatch to the ambient tmux server.

This is the seam and nothing else. It exists so an alternative transport — control mode, a recording, an in-memory fake — can be substituted without copying the library, which is what the downstream work currently has to do.

Changes by area

New: src/libtmux/engines/

  • base.py: CommandRequest (a tmux argv, without connection flags), CommandResult (the structured outcome), the TmuxEngine protocol, and three optional capability protocols — SupportsCommandLine (render the argv without running it, which is how the full command line reaches the debug log), SupportsConnection (marks an engine that dispatches over a named server and can be rebound), and SupportsTmuxVersion (report the tmux version, for callers rendering version-gated argv). Also CommandSeparator / is_command_separator, which mark an intentional command boundary so a ; passed as data can never become one.
  • connection.py: ServerConnection, the sole owner of the tmux binary path and connection flags. Derived from the server's public attributes on each use, so reassigning socket_name takes effect on the next command, and it memoizes its shutil.which lookup rather than re-walking $PATH.
  • subprocess.py: SubprocessEngine, the default.

Routed through the seam

  • src/libtmux/server.py: engine and connection properties, the engine= argument with validation at construction, and raise_if_dead() routed through dispatch.
  • src/libtmux/neo.py: fetch_objs() dispatches through the server's engine instead of building its own flags. Without this the object API never touches the engine, and an alternative engine cannot back sessions / windows / panes at all.
  • src/libtmux/common.py: tmux_cmd takes an engine= and is built from the engine's CommandResult. It reads process defensively, so an engine may return any structurally compatible result rather than only libtmux's own.

Design decisions

A protocol, not a base class. Structural typing means a third-party engine needs no import-time dependency on libtmux — it implements run and run_batch and is an engine. TmuxEngine's own method bodies are ..., so it is a shape to satisfy, not an implementation to inherit; a stateless engine writes run_batch as a loop over run.

run_batch stays on the protocol even though core never calls it. It is the override point where a persistent-connection engine pipelines instead of round-tripping per command. "Nothing calls it" is the reasoning that would delete the extension point the seam exists to provide.

tmux rejecting a command is data; never reaching tmux is an exception. A nonzero result sets returncode and stderr on the result object. Only a broken engine — missing binary, lost connection — raises.

cmd() keeps returning tmux_cmd. tmux_cmd is load-bearing in public annotations, so the compatibility path adapts the engine's CommandResult back into one rather than introducing a new return type. Returning CommandResult directly is a breaking change and is deliberately not part of this PR.

Arguments are not escaped. tmux treats a trailing ; on an argument as a command boundary, and libtmux has always relied on that parse. Fixing it is a real behavior change with a public-API consequence, so it belongs in its own PR rather than riding along with a seam.

Out of scope, by design

Each of these builds on the seam and can land independently: batching (cmd_batch), async engines, record/replay, name-based engine resolution and entry points, block-scoped engine swapping, control-mode codecs, argument escaping, CommandResult as the return of cmd(), and ok / raise_for_status result helpers.

Behavior change

One, filed under Breaking changes in CHANGES. Server.raise_if_dead() previously let tmux write its message straight to the terminal; routed through an engine, that text is captured onto the raised subprocess.CalledProcessError. The exception type is unchanged.

It is routed rather than left alone because otherwise the three flag builders still disagree — the defect this seam exists to fix — and a non-forking engine cannot have its liveness probe shelling out.

Verification

The default path is unchanged, which means the object-API tests must pass untouched:

$ git diff --stat origin/master...HEAD -- tests/test_server.py tests/test_session.py tests/test_window.py tests/test_pane.py tests/test_neo.py

No escaping machinery ships:

$ rg -n 'CommandSeparator|encode_direct_argv|split_direct_argv' src/ tests/ docs/

cmd() still returns the compatibility type:

$ rg -n -A 6 'def cmd\(' src/libtmux/server.py

Test plan

  • uv run mypy . — clean
  • uv run ruff check . and uv run ruff format . --check — clean
  • uv run pytest — full suite passes with zero edits to the object-API test modules
  • just build-docs — builds, including the new topic and API pages
  • test_flag_builders_agreecmd(), raise_if_dead() and fetch_objs() emit identical connection flags for a server with socket_name, config_file and colors set
  • test_unknown_color_raises_on_every_path — an unknown colors value raises on all three paths, matching Server.cmd()'s existing contract
  • test_server_drives_injected_engine_without_tmux — an injected engine backs cmd() with no tmux server running
  • test_process_is_popen_under_default_enginetmux_cmd.process reads as it did before the seam existed
  • test_server_drives_engine_returning_a_foreign_result — an engine returning a result type libtmux does not own drives cmd(); verified to fail without the defensive read
  • test_injected_engine_survives_mutation and test_default_engine_rebuilt_after_mutation — an injected engine is user-owned; the default one tracks socket_name changes

Related

The full engine feature set is #738. This branch is the strict subset of it that is only the seam, for review or landing ahead of the rest.

@tony
tony force-pushed the engine-seam-minimal branch from ed7206c to 18e3d72 Compare August 15, 2026 10:36
@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.10526% with 108 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.68%. Comparing base (036c521) to head (3de1062).

Files with missing lines Patch % Lines
src/libtmux/engines/connection.py 63.07% 24 Missing ⚠️
src/libtmux/engines/subprocess.py 50.00% 21 Missing and 1 partial ⚠️
src/libtmux/engines/base.py 44.44% 20 Missing ⚠️
src/libtmux/engines/instrumentation.py 60.00% 18 Missing ⚠️
src/libtmux/server.py 72.22% 14 Missing and 1 partial ⚠️
src/libtmux/common.py 80.64% 6 Missing ⚠️
src/libtmux/exc.py 66.66% 2 Missing ⚠️
src/libtmux/window.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #739      +/-   ##
==========================================
+ Coverage   52.37%   52.68%   +0.31%     
==========================================
  Files          26       30       +4     
  Lines        3729     3950     +221     
  Branches      747      761      +14     
==========================================
+ Hits         1953     2081     +128     
- Misses       1472     1568      +96     
+ Partials      304      301       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tony
tony force-pushed the engine-seam-minimal branch 2 times, most recently from b9deeb2 to 947eaa4 Compare August 15, 2026 12:32
tony added 4 commits August 22, 2026 17:41
why: Every tmux command forks the binary inline, so an alternative
transport -- control mode, a recording, an in-memory fake -- cannot be
substituted without copying the library, which is what the downstream
work had to do. Connection flags were built in three places that
disagreed, so config_file= and colors= reached tmux on some paths and
not others.

what:
- Route dispatch through a TmuxEngine protocol, defaulting to a
  subprocess engine that forks exactly as before
- Accept engine= on Server, and let an engine that names no server of
  its own adopt the server's connection rather than the ambient one
- Derive one ServerConnection for cmd(), raise_if_dead() and
  fetch_objs()
- Read the result's process field defensively, so an engine may return
  any structurally compatible result rather than only ours
- Mark intentional command boundaries with CommandSeparator, so a ";"
  passed as data can never become one
- Report a connection's tmux version behind SupportsTmuxVersion, for
  callers that render version-gated argv
- Keep cmd() returning tmux_cmd, and arguments reaching tmux
  unchanged, so the default path behaves as it did
why: A custom tmux_bin names a program, not a server. An engine built
with one and no -L/-S was treated as already knowing its server, so it
was left unbound and every command reached whichever tmux server a
flagless dispatch finds -- the silent ambient dispatch adoption exists
to prevent.

what:
- Add ServerConnection.names_server, asking whether a connection
  carries connection flags of its own; the engine side of adoption
  reads it instead of is_unconfigured, which keeps its server-side
  meaning of "carries nothing at all"
- Bind the server's flags onto such an engine while preserving the
  binary it was built with
- Document the binary-is-not-a-server rule on Server.engine and in
  the CHANGES deliverable prose
- Cover both adoption directions plus the two cases that already
  held, so a single-predicate regression cannot pass
why: The engine captures tmux's stderr instead of letting it reach the
terminal, and the raise then discarded it, so a caller was left with an
exit code and no way to recover what tmux said -- strictly less than
the message the terminal used to show.

what:
- Pass the captured stdout and stderr to CalledProcessError, matching
  what CompletedProcess.check_returncode raises
- Say so in the docstring and prove it in the doctest
- Assert the socket name reaches the exception, which holds across
  both wordings tmux uses for an unreachable server
why: TmuxEngine and SupportsCommandLine are runtime_checkable Protocols,
so isinstance() checks attribute names only -- never signatures, never
async-ness. An engine with `async def run` satisfied them, was accepted
by Server(engine=...), and failed on the first command with
`AttributeError: 'coroutine' object has no attribute 'cmd'`, naming
neither the engine nor the mismatch. An `async def command_line`
failed the same way, one line earlier, whenever DEBUG logging was on.

what:
- Guard every engine capability in one place, _guard_sync(), reached
  through the typed _dispatch_run() and _dispatch_command_line()
  wrappers, so a mistyped call site is a mypy error rather than a
  runtime AttributeError
- Collapse raise_if_dead onto self.cmd(), deleting the second dispatch
  site rather than guarding it twice
- Reject a declared-async member before calling it, so the common shape
  never creates a coroutine; test the returned value too, since a plain
  def can still hand one back
- Close a coroutine that did get created -- safe while unstarted, and
  suppressed against BaseException so a hostile awaitable cannot
  replace the diagnostic. Never cancel a Task or Future: one bound to
  another thread's loop would not receive it, and one shared with
  another awaiter would lose its result
- Let AsyncEngineMismatch escape the list-accessor leniency; a
  misconfigured engine is not a tmux failure and must not read as
  "no sessions"
- Add exc.AsyncEngineMismatch, naming the engine and the method, and
  document it on cmd() for Server, Session, Window and Pane
- Show the failure as a runnable example in docs/topics/engines.md

An eagerly-started Task (3.12+) has already run its body before run()
returns; the guard reports it but cannot undo it. Nothing dispatches
run_batch in-tree, so it gets no guard.
@tony
tony force-pushed the engine-seam-minimal branch from 0bcf5b9 to 740526e Compare August 23, 2026 00:16
tony added 12 commits August 22, 2026 19:53
why: A caller measuring engine traffic cannot tell a request that ran one
tmux command from one that inlined several into a single dispatch. The
distinction lives in the argv -- a CommandSeparator marks a real boundary
while a literal ";" is data -- and every engine already agrees on it
through is_command_separator. Leaving the arithmetic to each observer
invites them to count the encoded argv instead, where the separator has
been flattened to a plain string and the inlining is invisible.

what:
- Add command_count beside is_command_separator, returning the separators
  plus one, with doctests covering a group and a literal semicolon
- Export it from libtmux.engines
why: Counting or tracing tmux traffic meant patching subprocess.Popen from
outside, which under-reports any engine that does not fork and cannot see a
command group at all. TmuxEngine is a protocol, so the honest place for
observation is a decorator that satisfies the same protocol: a program that
wants none of it constructs none of it and runs the code it ran before, with
no guard on the hot path.

SQLAlchemy pays a boolean check per call for its event registry and Django
builds a context mapping even with no wrapper registered; both are shaped by
having concrete connection classes. A protocol lets the wrapper substitute
for the engine wherever one is accepted, and cost nothing where it is not.

what:
- Add Sink, a before/after/error observer surface matching the hooks
  OpenTelemetry and Sentry already target on SQLAlchemy, so an exporter
  written against one reads naturally here
- Add CountingSink, reporting requests, tmux commands, the commands that
  rode inside another request's argv, and elapsed time
- Count on request.args rather than the encoded argv, where an engine has
  flattened the separator and the inlining no longer shows
- Add InstrumentedEngine, which forwards anything the protocol does not
  cover to the engine it wraps
- Pin the substitution property as a test: the wrapper is a TmuxEngine
why: The module ships in libtmux.engines but the API page enumerated only
base, connection, and subprocess, so autodoc rendered no page for it and a
reader following the engines docs would not learn observation exists.

what:
- Give libtmux.engines.instrumentation its own section and automodule entry
- Record the deliverable in CHANGES, including why observation is composed
  rather than installed and what command_count distinguishes
why: Isolating a tmux server, naming a shape, and reducing samples to
percentiles are the same job whichever transport is being measured, and none
of it needs an engine -- only the classic Server the library already ships.
Keeping one copy per benchmark invites them to drift, and the isolation half
is exactly where drift costs: it encodes two tmux behaviours that were each
found the hard way.

new_server pins a keepalive session because killing a cell's session
otherwise drops the server to zero, and under tmux's exit-empty default the
next build can reach the socket mid-shutdown. reap_stale_scratch leaves a
directory alone while a tmux still answers on it, because stealing a
concurrent run's servers would be worse than leaking one directory.

what:
- Add scripts/bench/primitives.py with server isolation, shape parsing, the
  classic build, and the percentile summary, alongside the reasoning for the
  two behaviours above
- Return the reaped count rather than printing it, so the module needs no
  console and its caller keeps its own reporting
- Cover the shape, the statistics, the keepalive, and the reaper's refusal to
  touch a live directory
why: The seam's claim is that the default path is unchanged and that
observation costs nothing when nobody asks for it. Both are asserted in prose
and neither has a number behind it. A baseline is also what any later
transport has to be compared against, and there is currently nothing below
the engines that reports one.

what:
- Add scripts/bench/current_api.py, timing topology construction, the classic
  hierarchy read, and per-request dispatch through the seam
- Count requests separately from the tmux commands they carry, so a command
  group reads as two commands in one dispatch rather than as a saving; a
  change that merely moves cost between them stays visible as movement
- Report the sessions a shape built apart from the sessions the server holds,
  since the isolation keepalive is one of the latter and not one of the former
- Drive it through the shared primitives, so this benchmark and the engine
  benchmark above it measure with one ruler
…size

why: test_new_session_shell_env forwarded dict(os.environ) to new-session,
which becomes one -e KEY=VAL argument per variable. tmux sums every
argument and refuses past MAX_IMSGSIZE, so the test's outcome was a
function of whoever ran it: green on a lean CI runner, "command too long"
under a rich interactive shell, where the environment alone can exceed
16 KiB before libtmux appends its format string. The test asserts nothing
about the environment's contents, so the size was incidental.

what:
- Pass a small explicit environment, so the assertion is about what the
  test claims: window_command survives alongside environment=
- Cover the ceiling deliberately instead, with a payload built to exceed
  it and an assertion that the refusal is loud and creates no session
`--cov=./` measures every Python file in the tree, so the benchmark and
operations scripts under `scripts/` counted toward the library's coverage
figure. They are development tooling: their uncovered branches are argument
parsing, teardown paths, and failure reporting that exist to be read, not to
be exercised by the suite. Counting them made the reported number a statement
about the tooling's exercise rate rather than the library's.

The two patterns mirror the `tests/` entries directly above them, covering
`scripts/*.py` and one level of package beneath it, which is as deep as the
tree goes.

Verified by running one suite under both configurations:
`scripts/bench/primitives.py` contributes 70 statements without the omit and
disappears with it, taking the measured total from 3039 to 2969.
The benchmark declared `dependencies = ["libtmux"]` with no
`[tool.uv.sources]` entry, so running it the way its shebang says to --
`uv run --script scripts/bench/current_api.py` -- installed the released
libtmux into the ephemeral environment instead of this working tree. Since it
imports `libtmux.engines`, which no release carries, the documented invocation
ended at `ModuleNotFoundError` rather than at a wrong number. A script that did
import cleanly would have been worse: it would have reported the index's
timings as the working tree's.

Its existing tests could not see this. They load the module through
`spec_from_file_location` into the development environment, where `import
libtmux` succeeds regardless of what the inline block says, so the block was
never exercised by anything.

`tests/test_script_metadata.py` exercises the block directly, across every
script under `scripts/` rather than this one. It compares the pin by resolving
it against the script's own directory, so a script that moves between
directories fails unless its `..` count follows -- the failure mode a rename
invites. A scan-found-nothing guard keeps the parametrized checks from
reporting green on an empty set.

Verified: the documented invocation now builds and reports; removing the pin
and mis-spelling its depth each fail with the specific reason.
…nchmark

`new_server` promised a "fresh isolated server" and gave it a unique socket,
which isolates it from other servers and from nothing else. tmux reads
`~/.tmux.conf` when a server starts, so every measurement quietly carried
whatever the machine set -- a `history-limit`, a hook, a slow `default-shell`
-- and two machines could disagree about libtmux's cost for reasons that had
nothing to do with libtmux.

Demonstrated with a `HOME` holding a config that sets `history-limit 4242`:
before, the benchmark's server reported 4242; after, it does not. That is the
test, and it fails without the fix.

`config_file=os.devnull` is the same spelling the engine grid arrived at
independently for its own copy of this function, so the two now differ only in
whether the module globals they read carry a leading underscore.
… directory

`reap_stale_scratch` decided a scratch directory was abandoned by looking for a
tmux running out of it. A directory exists from the moment a run imports this
module, but its first server appears only at the first `new_server`, so during
that window every live run looked abandoned and was deleted -- socket directory
and all -- by any other run that happened to reap.

Under `pytest -n auto` the workers import at different moments, so the window is
always open somewhere. The failure surfaced far from its cause, as
`new-session: error creating /tmp/ltbench-XXXX/YYYY.sock (No such file or
directory)` in a worker whose directory had been removed underneath it, and as
`assert 'keepalive' in []` where the list accessor's empty-on-error contract
turned the missing socket into a silent empty list rather than a raised error.

Liveness is now the owning process, recorded in `owner.pid` when the directory
is created and checked with `os.kill(pid, 0)`. `PermissionError` counts as
alive: another user's process is running, merely not ours to signal. A directory
naming no owner -- one from before this file recorded them, or one caught
between creation and its claim -- is judged by age instead, and only then does
the tmux probe run. Every unknown resolves toward keeping the directory, so the
failure mode stays a leak rather than a theft.

The test that claimed to pin this only ever checked the reaper's own directory,
which is skipped by identity, so it could not have failed for the reason it
named; it is renamed to say what it actually pins. The four cases that needed a
rule are covered directly, and removing the owner check turns exactly the two
sparing tests red while the two reaping tests stay green.
`tests/` root is the library's namespace -- `tests/test_server.py` covers
`src/libtmux/server.py`, `tests/experimental/engines/` covers
`src/libtmux/experimental/engines/` -- and a top-level directory names the
repository tree it tests, which is what `tests/docs/` already does for `docs/`.
Tests for `scripts/` were sitting in the library's half of that namespace under
a `test_bench_`/`test_script_` prefix, so the prefix was doing a directory's
job and claiming the wrong tree while doing it.

    tests/test_bench_primitives.py  -> tests/scripts/bench/test_primitives.py
    tests/test_bench_current_api.py -> tests/scripts/bench/test_current_api.py
    tests/test_script_metadata.py   -> tests/scripts/test_metadata.py

Each of these finds the repository root by counting directories up from its own
file, so moving them deeper silently pointed every lookup at the wrong place --
the same counting mistake as a PEP 723 `path = ".."` that no longer reaches the
root. The counts move with the files.
`tests/*/test_*.py` reaches one directory below `tests/`, and `*` does not
cross a separator, so every test file nested deeper was measured as though it
were library code. Sixty-eight already were. Because those files execute, they
report near-total coverage and lift the figure rather than lowering it: the
suite reports 84% with them counted and 78% without, so 5,530 statements of
test code were worth six points of the library's score.

`**` matches any number of nested directories, so one pattern replaces the two
that enumerated levels, and a new directory cannot silently fall outside it.
The same collapse applies to the `scripts/` entries added alongside them, which
had the same one-level ceiling.

This is the third place in this tree where a path's depth was written down and
then stopped matching when a file moved -- the others being a PEP 723
`path = ".."` and the `parents[1]` lookups in the tests themselves. Each failed
silently rather than raising.

The reported figure drops six points because it stops averaging the library
with its own test suite.
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.

1 participant