refactor(adapters): let tmux _run carry output encoding and per-call env by dracic · Pull Request #41 · bmad-code-org/bmad-loop

dracic · 2026-07-02T21:36:39Z

What

Widens BaseTmuxBackend._run (src/automator/adapters/tmux_base.py) to expose the two knobs its docstring already advertised — output decoding and a per-call env — so a future native-Windows (psmux) leaf overrides zero lines of spawn plumbing.

_ENCODING: str | None = None class attribute → threaded as subprocess.run(..., encoding=self._ENCODING)
optional keyword-only env kwarg → threaded as subprocess.run(..., env=env)

Both default to today's behavior byte-for-byte.

Guarantees (POSIX/tmux unchanged)

_ENCODING defaults to None → subprocess.run(..., text=True, encoding=None) is identical to the current bare text=True (locale decoding). No POSIX read path changes.
env defaults to None → inherits the parent environment, exactly as today.
Both new params are keyword-only with defaults, so no existing call site or signature changes. _tmux and every current caller are untouched.

Tests

tests/test_multiplexer.py — existing tmux tests pass unmodified (the file is additive-only), plus three regression tests:

POSIX default path spawns with encoding=None and env=None (byte-identity)
a subclass setting _ENCODING="utf-8" reaches subprocess.run
a custom env is forwarded to the child without leaking into the parent process

ruff and black clean; full test_multiplexer + test_generic_tmux suites green.

Notes

Non-behavioral enablement — it shrinks a future native-Windows backend's override surface to near zero and is independently useful to anyone driving tmux from a non-UTF-8 locale, where the current locale-default decode can corrupt -F output.

Closes #40

Summary by CodeRabbit

Bug Fixes
- Improved tmux command execution to forward per-call environment variables and apply backend-controlled text encoding for reliable output decoding (including on Windows).
- Kept existing timeout and check behavior while ensuring the parent process environment remains unchanged.
Tests
- Added/updated tmux spawning “seam” tests to verify correct env and encoding propagation behavior.
Documentation
- Updated the tmux backend porting guidance to reflect the refined _run(..., env=...) contract and encoding handling expectations.

BaseTmuxBackend._run gains a class-level _ENCODING (default None) and an optional keyword-only env kwarg, both threaded into subprocess.run with defaults byte-identical to today's POSIX behavior. A native-Windows leaf can then force UTF-8 decoding and spawn with a scrubbed env without re-implementing the spawn primitive. The POSIX TmuxMultiplexer is unchanged at runtime (no encoding, no env passed); existing tmux tests pass unmodified, plus regression tests for the default path, a subclass setting _ENCODING, and a non-leaking custom env. Closes bmad-code-org#40

coderabbitai · 2026-07-02T21:37:11Z

Warning

Review limit reached

@pbean, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7a7753f3-0c9c-41de-98fb-94987ab86e41

📥 Commits

Reviewing files that changed from the base of the PR and between e5aa06a and 350589f.

📒 Files selected for processing (2)

src/automator/adapters/tmux_base.py
tests/test_multiplexer.py

Walkthrough

BaseTmuxBackend._run now accepts per-call env and a configurable encoding class attribute, and the tmux docs and tests were updated to match the new subprocess contract.

Changes

tmux _run seam extension

Layer / File(s)	Summary
Spawn contract and docs `src/automator/adapters/tmux_base.py`, `docs/porting-to-a-new-os.md`	Updates the tmux backend spawn seam to carry `_ENCODING` and keyword-only `env`, and revises the porting guidance to describe those responsibilities.
Spawn contract tests `tests/test_multiplexer.py`	Adds seam tests for default kwargs, subclass encoding overrides, and per-call env forwarding while preserving the parent process environment.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

bmad-code-org/bmad-auto#23: Introduces the shared tmux backend seam that this PR extends with encoding and env forwarding.
bmad-code-org/bmad-auto#26: Touches the same tmux subprocess contract and related seam tests.

Poem

I’m a rabbit with ears so keen,
Sniffing seams both crisp and clean.
UTF-8 hops through the glen,
env stays tidy now and then.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: extending tmux _run with output encoding and per-call env support.
Linked Issues check	✅ Passed	The PR implements the requested _run encoding and env plumbing, keeps defaults unchanged, and adds regression tests.
Out of Scope Changes check	✅ Passed	The docs and test updates are directly tied to the tmux _run seam change, with no unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

augmentcode · 2026-07-02T21:40:07Z

🤖 Augment PR Summary

Summary: Refactors the tmux spawn seam to support backend-controlled output decoding and per-call environment injection, primarily to reduce override surface for a future native-Windows backend.

Changes:

Adds BaseTmuxBackend._ENCODING and threads it into subprocess.run(..., encoding=...)
Extends BaseTmuxBackend._run with kw-only env and forwards it to subprocess.run
Updates OS porting guidance and adds regression tests for defaults, subclass encoding, and env forwarding/non-leakage

Technical Notes: Defaults (_ENCODING=None, env=None) preserve current POSIX behavior and avoid call-site changes.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-07-02T21:40:08Z

+        argv: list[str],
+        *,
+        check: bool = True,
+        env: dict[str, str] | None = None,


BaseTmuxBackend._run now accepts kw-only env, so any subclass override should accept env as well (even if unused) to avoid a future TypeError if callers start passing it; other locations where this applies: tests/test_multiplexer.py:263.

Severity: medium

Other Locations

tests/test_multiplexer.py:263

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

Addressed in e5aa06a — the PsmuxStyle test double now accepts env=None. (No production caller passes env yet, so this was latent, but worth closing now.)

…d _run seam Review fixups for bmad-code-org#41: - porting doc: decoding is the _ENCODING class attr and env a _run param; overriding _run itself is now only for binary/timeout - PsmuxStyle test double accepts the new env kwarg so it stays signature-compatible when a contract method starts forwarding env= - _run docstring: scrubbed envs must be built by removing vars from a copy of the parent env (Windows children need SystemRoot etc.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

pbean

Validated against main: the defaults are byte-identical on POSIX (encoding=None ≡ bare text=True, env=None ≡ inherit), no in-tree caller or override breaks (the only _run override is a test double), and the seam matches where the adapter work was already headed — a psmux leaf now sets _ENCODING/passes env= instead of re-implementing the spawn primitive. Full CI matrix (linux+windows, py3.11–3.14) green.

I pushed one review-fixup commit (e5aa06a) to the branch rather than blocking on nits:

docs/porting-to-a-new-os.md still described decoding as an _run() override — synced to the new seam
widened the PsmuxStyle test double to accept env= (also flagged by Augment) so it stays signature-compatible once a contract method forwards it
one docstring caveat: scrubbed envs should be built by removing vars from a copy of the parent env (Windows children need SystemRoot etc.)

Thanks for the clean, well-tested contribution — the issue #40 write-up made this easy to verify.

pbean · 2026-07-03T01:05:30Z

@CodeRabbit review
augment review

coderabbitai · 2026-07-03T01:05:42Z

@pbean I'll review the changes now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

augmentcode

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-07-03T01:08:16Z

 an eventual native-Windows "psmux" — can subclass :class:`BaseTmuxBackend` and
 override only the single spawn primitive :meth:`BaseTmuxBackend._run` (to tweak
-the binary / decoding / timeout) plus the few divergent methods (e.g. the
+the binary or timeout — output decoding and per-call ``env`` are ``_run``


src/automator/adapters/tmux_base.py:8-9: The docstring says output decoding is an _run parameter, but decoding is configured via the _ENCODING class attribute (there is no _run(..., encoding=...) kwarg). Consider rewording so readers don’t assume they can pass encoding directly to _run.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

Valid catch — the module docstring did claim decoding was a _run parameter. Fixed in 350589f: it now names _ENCODING as the class attribute and keeps env as the only per-call parameter.

augmentcode · 2026-07-03T01:08:16Z

+    monkeypatch.setattr(tmux_base.subprocess, "run", rec)
+    before = dict(os.environ)
+
+    scrubbed = {"PATH": os.environ.get("PATH", "")}  # e.g. nesting-guard vars dropped


tests/test_multiplexer.py:322: The test’s scrubbed env example builds an env from scratch with only PATH, which conflicts with the nearby _run docstring guidance to copy the parent env and remove vars (important on Windows for SystemRoot, etc.). It might be worth aligning this example with that guidance so it doesn’t get copied as a safe template.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

Agreed — the example contradicted the docstring guidance it sits next to. Fixed in 350589f: the test now sets a nesting-guard TMUX var and builds scrubbed by copying the parent env and deleting it, so the template models the safe pattern (and the forwarding assertion is no longer trivially parent-shaped).

…e env scrub Review fixups (round 2) for bmad-code-org#41: - module docstring: decoding is the _ENCODING class attribute, not a _run parameter — only env is; reword so nobody tries _run(..., encoding=...) - env-forwarding test: build the scrubbed env by copying the parent and removing the nesting-guard var, matching the _run docstring guidance, instead of rebuilding from scratch (unsafe template on Windows) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

pbean · 2026-07-03T01:21:39Z

augment review

augmentcode

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

augmentcode Bot reviewed Jul 2, 2026

View reviewed changes

pbean approved these changes Jul 3, 2026

View reviewed changes

augmentcode Bot reviewed Jul 3, 2026

View reviewed changes

pbean merged commit ecda7ac into bmad-code-org:main Jul 3, 2026
11 checks passed

dracic deleted the fix/gh-40-run-seam-encoding-env branch July 3, 2026 05:35

coderabbitai Bot mentioned this pull request Jul 3, 2026

Tmux-family base: factor the shell-dialect wrapper out of new_window/new_parked_window into overridable hooks #46

Closed

pbean mentioned this pull request Jul 3, 2026

chore(release): 0.8.0 #56

Merged

dracic mentioned this pull request Jul 3, 2026

Native-Windows psmux multiplexer backend: re-land as a thin leaf over the dialect seams #58

Open

Uh oh!

Conversation

dracic commented Jul 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Guarantees (POSIX/tmux unchanged)

Tests

Notes

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

augmentcode Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbean Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

pbean left a comment

Choose a reason for hiding this comment

Uh oh!

pbean commented Jul 3, 2026

Uh oh!

coderabbitai Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbean Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbean Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

pbean commented Jul 3, 2026

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dracic commented Jul 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

augmentcode Bot commented Jul 2, 2026 •

edited

Loading

augmentcode Bot Jul 2, 2026 •

edited

Loading

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading

augmentcode Bot Jul 3, 2026 •

edited

Loading

augmentcode Bot Jul 3, 2026 •

edited

Loading