Windows terminal-multiplexer backend#19
Draft
dracic wants to merge 1 commit into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a Windows psmux backend, platform-aware hook and process handling, POSIX path normalization in serialized outputs, and Windows CI/test coverage. Updates validation and run-stopping logic to use normalized status and PID identity checks. ChangesWindows Portability
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
🤖 Augment PR SummarySummary: This PR adds a native Windows terminal-multiplexer backend so the orchestrator can run on Windows without WSL. Changes:
Technical Notes: psmux window commands are executed via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/automator/install.py (1)
296-324: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize
seed_filesbefore writing git exclude patterns.The glob path now uses
rel.as_posix(), butseed_filesstill appends the rawrel. On Windows, a seed like.claude\settings.jsoncan be copied but excluded as/.claude\settings.json, which git ignore patterns won’t reliably match, risking seeded config being committed.Proposed fix
- seeded.append(rel) + seeded.append(Path(rel).as_posix())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/automator/install.py` around lines 296 - 324, Normalize the `seed_files` entries before they are added to `seeded` in `install.py`, matching the `seed_globs` behavior. The issue is that `seed_files` currently appends raw `rel` values, which can use backslashes on Windows and produce git exclude patterns that don’t reliably match. Update the `seed_files` loop so `seeded` stores a POSIX-style relative path, consistent with the `seed_globs` handling and the later git ignore generation.tests/test_psmux_probe.py (1)
359-467: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate the Claude probes on Claude actually being installed.
HAVE_PSMUXonly checks forpsmux/tmux/pwsh, but H7 asserts thatclaude.exeresolves as an application. On a host without Claude, H7 fails for the wrong reason, and H7-noprofile stops proving the wrapper-bypass path at all.Suggested fix
+HAVE_CLAUDE = shutil.which("claude") is not None +HAVE_CLAUDE_EXE = shutil.which("claude.exe") is not None +_LIVE_CLAUDE = pytest.mark.skipif( + not (HAVE_PSMUX and HAVE_CLAUDE), + reason="native Windows + psmux + claude required", +) +_LIVE_CLAUDE_EXE = pytest.mark.skipif( + not (HAVE_PSMUX and HAVE_CLAUDE_EXE), + reason="native Windows + psmux + claude.exe required", +) ... -@_LIVE +@_LIVE_CLAUDE_EXE def test_h7_claude_code_fix_tty_injection(): ... -@_LIVE +@_LIVE_CLAUDE def test_h7_noprofile_bypasses_fix_tty_wrapper():🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_psmux_probe.py` around lines 359 - 467, Gate the H7 Claude probe tests on Claude being installed, since HAVE_PSMUX only validates psmux/tmux/pwsh and not the Claude binary. Add a Claude availability check in the H7 setup path used by test_h7_claude_code_fix_tty_injection and test_h7_noprofile_bypasses_fix_tty_wrapper, and skip or short-circuit when claude.exe is not resolvable. Keep the existing assertions intact so they still verify the wrapper/injection behavior only when Claude is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 56: The checkout step currently leaves Git credentials persisted by
default, which is unnecessary for this job after repository code runs. Update
the actions/checkout step to explicitly disable credential persistence by
setting persist-credentials to false on the checkout configuration. Keep the
change localized to the checkout action invocation in the CI workflow.
In `@src/automator/adapters/profile.py`:
- Line 116: The project-relative path checks in the profile adapter still allow
parent traversal via values like ../..., so tighten the validation in the
profile path handling logic for hooks.config_path and skill_tree. Update the
relevant path-normalization/acceptance code in the profile adapter to reject any
relative path that escapes the project tree, not just absolute paths, and keep
the existing absolute-path rejection in place.
In `@src/automator/cli.py`:
- Around line 171-198: The Windows preflight in the CLI currently validates only
the hook relay, so `validate` can still succeed even when the selected backend
cannot start. Update the `get_multiplexer()`/`PsmuxMultiplexer` path in this
`win32` check to also verify the backend prerequisites (`psmux`, `tmux`, and
`pwsh`) before reporting success, and add clear `problems.append(...)` messages
when any dependency is missing or unusable.
In `@src/automator/plugins/manifest.py`:
- Around line 33-35: The path validation in _check_relative_paths only rejects
empty or absolute values, so manifest-relative entries can still contain ..
segments and escape the plugin/project root. Update _check_relative_paths (and
the related validation path used for seed_files, seed_globs, and
[python].module) to explicitly reject any value containing parent-directory
traversal, alongside the existing absolute-path check, so only safe relative
paths are accepted.
In `@src/automator/runs.py`:
- Around line 227-256: The stop path in stop_run is still trusting the current
process owning the PID instead of the engine that originally wrote the run
state, so a recycled PID on Windows can be targeted by terminate_pid. Update the
run metadata written at start-up to persist the engine’s pid_identity alongside
the raw PID, and change the guard in stop_run to compare the stored identity
against the live process before calling terminate_pid or kill_pid_forced. Use
the existing helpers read_pid, pid_identity, and _same_pid_alive as the main
touchpoints for wiring this through.
In `@tests/test_engine_worktree.py`:
- Around line 537-553: The hook command serialization in hook_cmd and the
plugin.toml assembly currently uses TOML literal strings for cmd, which breaks
when the launcher path contains an apostrophe. Update the command generation in
this test helper to emit cmd as a TOML basic string instead, keeping the
launcher path safely quoted so plugin.toml remains valid across paths with
special characters.
In `@tests/test_psmux_probe.py`:
- Around line 242-274: In test_h3_posix_parked_trailer_non_viable, avoid
asserting that the POSIX sh -c parked trailer never runs when sh is available on
the host, since that makes the probe depend on local configuration rather than
the backend. Gate the native sh_ran expectation on sh_available (or otherwise
skip/soften that check when sh.exe is present), while keeping the pwsh
$LASTEXITCODE path as the real assertion for the backend behavior. Keep the
_record output informative by reporting both sh availability and the pwsh
result.
---
Outside diff comments:
In `@src/automator/install.py`:
- Around line 296-324: Normalize the `seed_files` entries before they are added
to `seeded` in `install.py`, matching the `seed_globs` behavior. The issue is
that `seed_files` currently appends raw `rel` values, which can use backslashes
on Windows and produce git exclude patterns that don’t reliably match. Update
the `seed_files` loop so `seeded` stores a POSIX-style relative path, consistent
with the `seed_globs` handling and the later git ignore generation.
In `@tests/test_psmux_probe.py`:
- Around line 359-467: Gate the H7 Claude probe tests on Claude being installed,
since HAVE_PSMUX only validates psmux/tmux/pwsh and not the Claude binary. Add a
Claude availability check in the H7 setup path used by
test_h7_claude_code_fix_tty_injection and
test_h7_noprofile_bypasses_fix_tty_wrapper, and skip or short-circuit when
claude.exe is not resolvable. Keep the existing assertions intact so they still
verify the wrapper/injection behavior only when Claude is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee16d245-5d50-4e7f-a350-0802a9bbfa10
📒 Files selected for processing (36)
.github/workflows/ci.ymlsrc/automator/adapters/multiplexer.pysrc/automator/adapters/profile.pysrc/automator/adapters/psmux_backend.pysrc/automator/adapters/tmux_backend.pysrc/automator/cli.pysrc/automator/data/plugins/unity/unity_plugin.pysrc/automator/engine.pysrc/automator/install.pysrc/automator/model.pysrc/automator/platform_util.pysrc/automator/plugins/manifest.pysrc/automator/resolve.pysrc/automator/runs.pysrc/automator/sweep.pysrc/automator/tui/data.pysrc/automator/verify.pytests/conftest.pytests/test_cli.pytests/test_decisions.pytests/test_engine.pytests/test_engine_plugin.pytests/test_engine_worktree.pytests/test_generic_tmux.pytests/test_hook_bus.pytests/test_install.pytests/test_platform_util.pytests/test_portability_guard.pytests/test_psmux_backend.pytests/test_psmux_live.pytests/test_psmux_probe.pytests/test_runs.pytests/test_sanitize.pytests/test_tui_app.pytests/test_tui_launch.pytests/test_verify.py
Author
|
Some win32 workarounds could perhaps be handled more elegantly, and I still have some tests named after the tea testing project that I should rename and clean up. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cli.py (1)
766-782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeed these failure-path tests with the same project bootstrap as the success case.
Unlike the success-path test on Lines 740-758, these two tests skip config/init setup, so they only stay green while
validatehappens to hit the Windows preflight before project checks. That makes them order-dependent instead of focused on the specific failure being asserted.Also applies to: 802-812
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 766 - 782, The failure-path tests for validate are missing the same project bootstrap used by the success case, so they rely on Windows preflight happening before any project checks and become order-dependent. Update the affected tests to use the same initialized project setup as the success-path validate test before invoking cli.main, while keeping the psutil-missing assertion focused on the validate path; apply the same fix to the other related test around the validate bootstrap helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 766-782: The failure-path tests for validate are missing the same
project bootstrap used by the success case, so they rely on Windows preflight
happening before any project checks and become order-dependent. Update the
affected tests to use the same initialized project setup as the success-path
validate test before invoking cli.main, while keeping the psutil-missing
assertion focused on the validate path; apply the same fix to the other related
test around the validate bootstrap helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff1fa157-d5ce-4151-9134-b29317ddb00c
📒 Files selected for processing (13)
.github/workflows/ci.ymlsrc/automator/adapters/profile.pysrc/automator/cli.pysrc/automator/install.pysrc/automator/platform_util.pysrc/automator/plugins/manifest.pytests/test_cli.pytests/test_decisions.pytests/test_engine_worktree.pytests/test_platform_util.pytests/test_probe.pytests/test_psmux_backend.pytests/test_psmux_probe.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_decisions.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/ci.yml
- src/automator/cli.py
- src/automator/plugins/manifest.py
- src/automator/adapters/profile.py
- src/automator/install.py
- tests/test_psmux_backend.py
- tests/test_platform_util.py
- tests/test_psmux_probe.py
Collaborator
Review — thank you, this is strong work 🙏First off: this is a genuinely high-quality contribution. The psmux backend is careful, the The main ask is a rebase onto 1. Core edits that the new seams now supersede — please drop on rebase
2. The one real rebase gotcha: the
|
dracic
pushed a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 29, 2026
The kill/liveness primitives branched on sys.platform inline in platform_util, and stop_run had no way to force-kill a wedged engine — so a native-Windows backend (PR bmad-code-org#19) would have to edit core. Introduce a ProcessHost seam (terminate / force_kill / is_alive / identity) with per-OS impls and a name-keyed registry mirroring the multiplexer backend selection, so adding an OS is a new subclass + one registration line. stop_run now escalates: an engine that ignores SIGTERM past the grace window is SIGKILL'd — but only while host.identity(pid) still matches the value recorded at stop-time (a pid-reuse guard); otherwise it raises the new StopRunError rather than risk an unrelated process. This is a deliberate POSIX behavior change, pinned by new tests. The graceful-first / single-writer-of- stopped invariant is unchanged. platform_util keeps terminate_pid/pid_alive as thin back-compat shims over the host; detach_kwargs stays put. The TUI worker widens its except to surface StopRunError/ProcessHostError. The portability guard swaps platform_util for process_host in KILL_PROBE_ALLOW and allows its Linux /proc identity read. Unity teardown de-dup is deferred: unity_teardown.py runs under a bare python3 and can't reliably import the package. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dracic
pushed a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 29, 2026
The hook-registration sites in install/probe hardcoded `python3`, and `cmd_validate` hardcoded a `tmux` PATH check — two more axes a native backend (psmux PR bmad-code-org#19) would have to branch on `sys.platform` to cross. Route both through the platform-selected seams so a new OS registers rather than edits core: - Add `ProcessHost.hook_interpreter()` (POSIX `python3`; Windows `uv run --no-project python`); install/probe interpolate it instead of a `python3` literal. POSIX output is byte-for-byte unchanged. - Extract `_platform_preflight()` in cli.py: ask the multiplexer backend (`available()`/`version()`) and name the process host, replacing the hardcoded `tmux` probe in `cmd_validate`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dracic
pushed a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 29, 2026
Spec-frontmatter status gates compared the raw `status:` value, so a hand-edited spec carrying a stray-cased `Done`/`In-Review` silently failed to advance a story. Add `verify.status_of()` (strip + lower) and route the six spec-frontmatter status reads through it (verify dev/review + bundle gates, engine post-dev sync, sweep bundle). The template and sprint-status tokens are lowercase, so behavior is unchanged for well-formed specs; `devcontract` keeps its own lowercasing (it parses skill prose). Also fix the manual-rollback notice: an empty baseline rendered an invalid `git reset --hard the run's baseline commit` — use a `<baseline_commit>` placeholder instead. Cross-cutting fix surfaced by PR bmad-code-org#19; landed independently of the psmux work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dracic
pushed a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 29, 2026
The profile- and plugin-manifest "must be project-relative" guards used `Path(value).is_absolute()`, which is platform-dependent (on Windows a POSIX-absolute `/etc/passwd` reads as *not* absolute) and never caught relative `..` escapes — so `../../etc` and cross-OS-absolute paths slipped through. Add `platform_util.is_absolute_path` (absolute in either POSIX or Windows terms) and `has_parent_ref` (a `..` segment in either flavor) and pair them at every guard site in adapters/profile.py and plugins/manifest.py. Hardening surfaced by PR bmad-code-org#19; landed independently of the psmux work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dracic
pushed a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 29, 2026
A worktree task's `spec_file` (state.json) and the resolve context's `resolution_path` were serialized via `str(Path)`, which emits backslashes on Windows — so a state/context file written on one OS read back with OS-specific separators. Persist both via `as_posix()` so the cross-OS state contract is a single forward-slash string everywhere (a no-op on POSIX). Cross-OS portability surfaced by PR bmad-code-org#19; landed independently of the psmux work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dracic
pushed a commit
to dracic/bmad-auto
that referenced
this pull request
Jun 29, 2026
The data layer caches a parse while a file's (mtime_ns, size) is unchanged. On a coarse-mtime filesystem (e.g. WSL2 drvfs) a same-size rewrite within one mtime tick is invisible to that signature, so the dashboard serves a stale parse. The engine rewrites state.json atomically (temp + os.replace), so every write lands on a fresh inode — fold st_ino into _stat_sig to catch the change regardless of mtime resolution. Surfaced by PR bmad-code-org#19; landed independently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dracic
force-pushed
the
feat/psmux-backend
branch
2 times, most recently
from
47e3bfd to
c75e34d
Compare
Author
|
Rebased psmux-only on top of 0.7.7 and force-pushed (single commit) - cross-cutting non-psmux fixes dropped as discussed; CI-only fixups since last push: generic probe-test names, On Windows, os.kill(pid, 0) is not a harmless liveness probe as on POSIX - signal 0 maps to CTRL_C_EVENT, which injected phantom KeyboardInterrupts into pytest and crashed the session, so PID-liveness must use a Windows-safe check (psutil/OpenProcess) instead. Suite green on Linux + Windows. |
dracic
force-pushed
the
feat/psmux-backend
branch
from
c75e34d to
645c8cd
Compare
dracic
force-pushed
the
feat/psmux-backend
branch
from
645c8cd to
fbb485f
Compare
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.
What
Adds a native-Windows terminal-multiplexer backend (
PsmuxMultiplexer) so the orchestrator runs on a native Windows host (psmux/ConPTY) without WSL, alongside the existing tmux backend which stays the default on POSIX.Why
The transport was tmux-only, so native Windows was unsupported. psmux is a protocol-compatible tmux drop-in on Windows, but several behaviours diverge (UTF-8 output, no POSIX
shfor the parked-window trailer, env propagation, agent-teams auto-injection) and the process/stop layer had Windows branches that were never exercised.How
adapters/psmux_backend.pyimplements theTerminalMultiplexercontract, overriding only the genuinely-divergent methods (pwsh parked-window trailer, UTF-8 subprocess decoding, env injection, teammate-mode isolation) and inheriting the rest.adapters/tmux_backend.py: extracted a small_run()subprocess seam (shared timeout) purely so the psmux backend can subclass and addencoding="utf-8". POSIX command strings are unchanged byte-for-byte (pinned bytest_portability_guard.py); no behavioural change on Linux/macOS.adapters/multiplexer.py:get_multiplexer()now picks psmux onwin32, else tmux (WSL reportslinuxand correctly keeps tmux). This is the policy seam that was already documented as the selection point.platform_util.py+runs.py: addedkill_pid_forced(Wintaskkill /F /T, POSIXSIGKILL),pid_identity(psutil start-time) and zombie-aware liveness;stop_runnow escalates a non-cooperating process to a forced kill after the graceful window and identity-guards both the polite and forced paths against PID reuse. The engine stays the single writer ofstopped.install.py,cli.py: hook command branches touv run --no-project pythonon Windows (no reliablepython3on PATH);validatepreflights psmux + uv presence. Cross-platform absolute-path check (is_absolute_path) replacesPath.is_absolute()inadapters/profile.py..github/workflows/ci.yml: adds a native-Windows runner so the Windows paths are exercised in CI.Testing
pytestgreen on Linux and native Windows; new suites cover the psmux backend, the process layer, and live psmux probes (test_psmux_backend.py,test_platform_util.py,test_psmux_probe.py,test_psmux_live.py), withtest_portability_guard.pyenforcing that the POSIX backend leaks no Windows-isms and vice-versa. Live end-to-end was human-validated on a real Windows+psmux host (hook env injection, engine-written graceful stop, session teardown).This PR is for detailed testing and review only — please do not merge as-is. The native-Windows paths need broader validation across psmux versions and host configurations (the backend targets a fast-moving psmux that emulates tmux 3.3.6). Treat it as a working reference for evaluating the approach and exercising the Windows transport end-to-end, pending sign-off after thorough testing.
Summary by CodeRabbit