refactor(adapters): extract the shell-dialect seam from new_window/new_parked_window#47
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR factors tmux shell-dialect handling into overridable hooks, updates ChangesTmux shell dialect seam
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 refactors tmux window creation to separate the shell-dialect-specific pieces from the shared tmux argv/protocol scaffolding. Changes:
Technical Notes: The intent is to enable non-POSIX / native-Windows tmux-family backends to swap shell dialect details without duplicating full contract method bodies, reducing adapter drift risk. 🤖 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.
🧹 Nitpick comments (1)
tests/test_multiplexer.py (1)
345-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise shell quoting in the byte-identity fixture.
This test would still pass if
_join_argvregressed fromshlex.join(argv)to a naive" ".join(argv). Use an argv containing spaces or shell metacharacters so the POSIX byte-identity check also locks the quoting contract.🧪 Proposed test strengthening
_PARKED_SH_SOURCE = ( - 'echo hi; ec=$?; echo "[bmad-auto exited $ec — press enter]"; read -r; ' + "echo 'two words' 'semi;colon'; ec=$?; " + 'echo "[bmad-auto exited $ec — press enter]"; read -r; ' "ret=$(tmux show-options -wqv %3 2>/dev/null); " 'if [ "$ret" = "detach" ]; then tmux detach-client 2>/dev/null; ' 'elif [ -n "$ret" ]; then ' 'tmux switch-client -t "$ret" 2>/dev/null || tmux switch-client -l 2>/dev/null; fi' ) @@ - TmuxMultiplexer().new_parked_window("s", "n", tmp_path, ["echo", "hi"], "%3") + TmuxMultiplexer().new_parked_window( + "s", "n", tmp_path, ["echo", "two words", "semi;colon"], "%3" + )🤖 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_multiplexer.py` around lines 345 - 376, The byte-identity check in test_new_parked_window_posix_argv_byte_identical is too weak because it still passes with naive argv joining. Strengthen the fixture by using an argv in TmuxMultiplexer.new_parked_window that includes spaces or shell metacharacters, and update the expected _PARKED_SH_SOURCE assertion so the test verifies correct shlex-style quoting rather than plain string concatenation.
🤖 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_multiplexer.py`:
- Around line 345-376: The byte-identity check in
test_new_parked_window_posix_argv_byte_identical is too weak because it still
passes with naive argv joining. Strengthen the fixture by using an argv in
TmuxMultiplexer.new_parked_window that includes spaces or shell metacharacters,
and update the expected _PARKED_SH_SOURCE assertion so the test verifies correct
shlex-style quoting rather than plain string concatenation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fcd55cc-0106-46b9-b1fb-343427e344f8
📒 Files selected for processing (2)
src/automator/adapters/tmux_base.pytests/test_multiplexer.py
…w_parked_window The tmux argv construction, parked-window protocol, and recipe shape stay in BaseTmuxBackend once; the shell-dialect fragments (exit capture, echo, park, argv join, trailer, source prefix, shell wrap, window launch) become overridable hooks with POSIX-sh defaults. POSIX output is byte-identical, locked by tests asserting the literal subprocess argv; a test leaf proves a non-POSIX dialect overrides only hook strings, never a contract method body.
dracic
force-pushed
the
refactor/gh-46-shell-dialect-seam
branch
from
ed87eb3 to
5bb6937
Compare
… guides
Review follow-up: the module docstring claimed a non-POSIX leaf never edits
a contract method body, but pipe_pane still hands tmux a POSIX `cat >>`
redirection — scope the claim to new_window/new_parked_window and name
pipe_pane as the remaining method-body override. Update
adapter-authoring-guide.md and porting-to-a-new-os.md, which still described
the pre-seam model ('override the few divergent methods, e.g. the
parked-window trailer'), to point at the shell-dialect hooks instead.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed — the seam is sound: POSIX argv/source verified byte-identical, the leaf overrides nothing, all call sites unaffected. Pushed
No code-behavior changes; |
2 participants
What
BaseTmuxBackend.new_window/new_parked_windowno longer hard-code the POSIX shell wrapper: the tmux argv construction, parked-window protocol, and recipe shape stay in the base once, while the shell-dialect fragments become small overridable hooks with POSIX-sh defaults.Why
A native-Windows tmux-family leaf (follow-up to #40) has no
/bin/sh; with the wrapper hard-coded it had to duplicate both entire methods just to swap the shell dialect — the biggest drift risk in the adapter layer.Fixes #46
How
BaseTmuxBackend:_EXIT_CAPTURE/_ECHO/_PARKfragments,_join_argv,_parked_trailer,_source_prefix,_shell_wrap,_window_launch— each maps 1:1 to a divergence a non-POSIX leaf actually needs; the recipe line itself is dialect-neutral (;sequencing and"$ec"interpolation are common across the family).new_window/new_parked_windowto compose from the hooks; the POSIX leaf (tmux_backend.py) overrides nothing and its output is byte-identical._EXIT_CAPTUREoverride must bindec;_source_prefixmust self-terminate with"; "; which hooks a non-POSIX leaf must override). No non-POSIX implementation lands here — this only opens the seam.Testing
New tests lock the exact pre-refactor
subprocess.runargv (including the fullsh -csource literal) for both methods, and a fake-dialect test leaf proves overriding only the hooks keeps the base scaffolding intact.tests/test_multiplexer.py11/11; full suite green on Windows against the known baseline (pytest -k "not real_signal", 1322 passed).Summary by CodeRabbit
Bug Fixes
Tests
Documentation