Add configurable tmux/Zellij multiplexer backends - #132
Open
sysadmin-metrum-ai wants to merge 4 commits into
Open
Add configurable tmux/Zellij multiplexer backends#132sysadmin-metrum-ai wants to merge 4 commits into
sysadmin-metrum-ai wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR implements a pluggable multiplexer abstraction that enables Tutti to control terminal sessions through either tmux or Zellij, selected via configuration. The codebase transitions from hardcoded tmux commands to a trait-based backend system with dual implementations, refactoring the session layer to delegate to the selected backend at runtime. ChangesMultiplexer abstraction and dual-backend support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@CHANGELOG.md`:
- Around line 3-11: There are duplicate "## Unreleased" sections in CHANGELOG.md
causing split release notes; consolidate them into a single "## Unreleased"
heading by moving the entries listed (the Added items about multiplexer support,
Zellij-backed paths, and Integration coverage) under the existing Unreleased
block and remove the duplicate heading and its empty/redundant lines so all
unreleased notes live in one place.
In `@README.md`:
- Line 453: Update the README.md CI description to use the correct platform
capitalization "GitHub" (replace any occurrences like "Github" or "github" in
the CI smoke profile line that reads "CI smoke profile
(`.github/workflows/ci.yml`) runs headless `tt doctor --strict` + `tt run
smoke-check --strict`" so it consistently uses "GitHub" in documentation.
- Line 559: Update the credit line mentioning Zellij multiplexer support (the
string "Zellij multiplexer support is credited to Chetan Gadgil
(`chetan@metrum.ai`), Metrum AI Inc, 2006.") to use the correct year—replace
"2006" with "2026" so the credit reads with the appropriate timeline.
In `@src/config/mod.rs`:
- Around line 78-98: Rename the public config types and field to use the
project's musical terminology: change OrchestratorConfig to ArrangementConfig,
MultiplexerType to ConductorType, MultiplexerConfig to ConductorConfig, and the
field multiplexer_type to conductor_type (keep inner variant names like
Tmux/Zellij but update any TmuxMultiplexerConfig/ZellijMultiplexerConfig type
names to TmuxConductorConfig/ZellijConductorConfig if present). Update all serde
derives/rename annotations and Default/Clone/Serialize/Deserialize usages and
any code referencing OrchestratorConfig, MultiplexerType, MultiplexerConfig,
multiplexer_type (and the tmux/zellij config types) to the new names so the
public config keys and types follow the "arrangements/voices/movements/phrases"
convention.
In `@src/multiplexer/mod.rs`:
- Around line 12-15: Rename the public struct field target_agent in
SessionMetadata to target_voice and update any related public APIs or types that
reference it (e.g., constructors, serializers, deserializers, tests, and
callsites) to the musical terminology; specifically, change the field name in
the SessionMetadata definition and then find/replace usages of
SessionMetadata.target_agent to SessionMetadata.target_voice, update any
JSON/serde keys or conversion code that expose the name, and adjust function
signatures and documentation to use target_voice so the new name is consistent
across the codebase.
- Around line 52-69: The code silently ignores a poisoned CURRENT_CONFIG mutex
(in set_current_config and current_backend), causing updates to be dropped and
current_backend to fallback to the default runtime; change both places to handle
PoisonError explicitly by recovering the inner guard (use
lock().unwrap_or_else(|poison| poison.into_inner()) or equivalent) and log or
surface the poison event instead of swallowing it — in set_current_config use
that recovered guard to assign the new RuntimeMultiplexerConfig
(RuntimeMultiplexerConfig, set_current_config, CURRENT_CONFIG), and in
current_backend avoid using unwrap_or_default() to fall back to
RuntimeMultiplexerConfig::default(); instead recover the guard and use it to
build the multiplexer via create_multiplexer(&runtime) or return/panic with a
clear error so commands aren’t routed to the wrong backend (current_backend,
create_multiplexer).
In `@src/multiplexer/tmux.rs`:
- Around line 199-241: In send_text_via_tmux_buffer, delete the temporary server
buffer (buffer_name) after a successful paste to avoid accumulating buffers:
after the paste-buffer call succeeds (after paste_output is checked) run tmux
delete-buffer -b <buffer_name> (use Command::new("tmux").args([...]).output() or
spawn()/wait), check its exit status similar to load/paste and return a
command_error on failure (or at minimum log/ignore failures consistently), then
continue to return Ok(()). Ensure you reference the existing buffer_name
variable and use the same command_error helper used for
load-buffer/paste-buffer.
- Around line 61-66: The code is interpolating env var names directly into a
shell export, which can break or inject shell syntax; update the loop that uses
env_vars, should_strip_inherited_env_var, shell_escape_value, and
self.send_text(&meta.session_id, &export_cmd) to first validate each key against
a POSIX-safe identifier regex (e.g. ^[A-Za-z_][A-Za-z0-9_]*$), skip or log any
invalid names instead of exporting them, and only build/send the export command
for keys that pass validation so that malformed keys cannot be injected into the
shell command.
In `@src/multiplexer/zellij.rs`:
- Around line 250-255: The loop in build_launch_script blindly interpolates env
var names into shell code (lines.push(format!("export {}={}", key, ...))) which
can break the shell or allow injection; add validation to ensure keys match a
safe identifier pattern (e.g. /^[A-Za-z_][A-Za-z0-9_]*$/) before emitting an
export, and skip (or log and skip) any key that fails validation. Implement a
small helper like is_valid_env_key(key: &str) and call it alongside
should_strip_inherited_env_var, or alternatively sanitize/reject keys in that
spot, keeping shell_escape_value for the values.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b9bc83a9-97e6-4bba-9546-8ba8b0514079
📒 Files selected for processing (18)
CHANGELOG.mdREADME.mdsrc/automation/mod.rssrc/budget/mod.rssrc/cli/doctor.rssrc/cli/handoff.rssrc/cli/up.rssrc/cli/watch.rssrc/config/defaults.rssrc/config/mod.rssrc/error.rssrc/main.rssrc/multiplexer/mod.rssrc/multiplexer/tmux.rssrc/multiplexer/zellij.rssrc/session/tmux.rssrc/state/mod.rstests/send_auto_up_integration.rs
Author
|
@coderabbitai review Follow-up fixes pushed:
Kept |
|
Noted on keeping ✅ Actions performedReview triggered.
|
Author
|
Maintainer note: this PR is locally verified and the current CodeRabbit status check is passing. The remaining blockers appear administrative: fork PR CI is |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
462-464:⚠️ Potential issue | 🟠 MajorUpdate docs:
layout_pathunder[multiplexer.zellij]isn’t wired in the Zellij backend.
themeis implemented insrc/multiplexer/zellij.rsvia theZELLIJ_THEMEenvironment variable.layout_pathis defined insrc/config/mod.rsand appears in config examples/tests, but there are no code references insrc/multiplexer/that would cause it to affect the launched Zellij session—so the README documents a no-op field.🤖 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 `@README.md` around lines 462 - 464, The README documents a no-op `layout_path` under `[multiplexer.zellij]` because the value declared in src/config/mod.rs isn’t passed into the Zellij launcher in src/multiplexer/zellij.rs (only `ZELLIJ_THEME` is used there), so either wire the config through or update docs: read the `layout_path` property from the multiplexer config struct (the field declared in src/config/mod.rs) in the Zellij launcher code in src/multiplexer/zellij.rs and pass it to the spawned Zellij process (via the appropriate CLI flag or environment variable, similar to how `ZELLIJ_THEME` is applied) so the configured layout actually affects the launched session, or remove/mark `layout_path` as unused in the README if you prefer not to implement it yet.
🤖 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.
Outside diff comments:
In `@README.md`:
- Around line 462-464: The README documents a no-op `layout_path` under
`[multiplexer.zellij]` because the value declared in src/config/mod.rs isn’t
passed into the Zellij launcher in src/multiplexer/zellij.rs (only
`ZELLIJ_THEME` is used there), so either wire the config through or update docs:
read the `layout_path` property from the multiplexer config struct (the field
declared in src/config/mod.rs) in the Zellij launcher code in
src/multiplexer/zellij.rs and pass it to the spawned Zellij process (via the
appropriate CLI flag or environment variable, similar to how `ZELLIJ_THEME` is
applied) so the configured layout actually affects the launched session, or
remove/mark `layout_path` as unused in the README if you prefer not to implement
it yet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8a21fb67-28a9-4f2a-a771-3f4fc0f9a207
📒 Files selected for processing (6)
README.mdsrc/config/mod.rssrc/multiplexer/mod.rssrc/multiplexer/tmux.rssrc/multiplexer/zellij.rssrc/session/tmux.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/session/tmux.rs
- src/multiplexer/zellij.rs
- src/multiplexer/mod.rs
- src/multiplexer/tmux.rs
- src/config/mod.rs
Collaborator
Hermes PR review trackerStatus
Findings
Evidence checked
Recommended next actionOwner: Tutti multiplexer lane. Keep sequenced behind #133 unless Adam changes merge order; rebase/update from NotesNo merge/deploy/close action taken. Tutti remains limited to its self-dogfood pilot. Managed by Hermes. Last updated: |
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.
Maintainer Attention
This PR is locally verified and the current CodeRabbit status check is passing, but GitHub still reports the PR as blocked because:
action_requiredand needs a maintainer to approve/run itCHANGES_REQUESTEDreview remains attached even though follow-up commits resolved the substantive comments and the latest CodeRabbit check is greenLatest local verification:
cargo test --quietpassed with 460 unit tests and 2 integration tests.Summary
[orchestrator].multiplexer_typewith backwards-compatibletmuxdefault andzellijsupportVersioning
UnreleasedVerification
cargo test --quietsend --auto-up,peek, anddowntt run verify-zellij --strictensure_running, prompt send/wait,tt peekmarker verification, and cleanupNotes
The Zellij multiplexer path includes provenance from downstream operator use in the Metrum AI fork, now documented without a prominent personal callout.
Summary by CodeRabbit
New Features
tutti.toml.Documentation
tt doctorvalidation for multiplexer availability.