lhumina_code/hero_router

Fork 0

Terminal: remember tmux layout and state between sessions #70

New issue

Closed

opened 2026-04-29 11:14:49 +00:00 by mahmoud · 4 comments

mahmoud commented

2026-04-29 11:14:49 +00:00

Owner

Problem

When a user reconnects to the hero_proc terminal, their tmux layout
and state is not remembered. Users have to reorganize their windows
every time they reconnect.

Requirements

Terminal should persist and restore tmux session state on reconnect
If a tmux session already exists for the user, attach to it
instead of creating a new one
Layout (splits, window names) should be preserved

Relevant code

crates/hero_proc_ui/ — terminal UI
PTY/WebSocket attach logic

Acceptance Criteria

Reconnecting attaches to existing tmux session
Window layout is preserved between sessions
New session only created if none exists

## Problem When a user reconnects to the hero_proc terminal, their tmux layout and state is not remembered. Users have to reorganize their windows every time they reconnect. ## Requirements - Terminal should persist and restore tmux session state on reconnect - If a tmux session already exists for the user, attach to it instead of creating a new one - Layout (splits, window names) should be preserved ## Relevant code - `crates/hero_proc_ui/` — terminal UI - PTY/WebSocket attach logic ## Acceptance Criteria - [ ] Reconnecting attaches to existing tmux session - [ ] Window layout is preserved between sessions - [ ] New session only created if none exists

ashraf commented

2026-05-03 11:52:28 +00:00

Member

Implementation Spec for Issue #70

Objective

Make tmux terminal sessions in hero_router persist their layout and window state across browser disconnects and PTY-job restarts, by switching the launch command from a bare tmux (which always creates a fresh session) to tmux new-session -A -s <name> (attach if exists, else create), keyed by the user-facing session name.

Requirements

When a tmux PTY job (re)starts for a hero_router session whose name is N, it MUST attach to a tmux daemon-side session named tmux_session_name(N) if one already exists, instead of creating a brand-new one.
A new tmux daemon-side session MUST be created only when none with that name is found.
Window/pane layout, window names, and shell history of attached panes are inherently preserved by the tmux server across client disconnects, so this single change satisfies the layout-preservation requirement.
Naming convention: tmux_session_name(N) = the hero_router session name with every / replaced by _. Hero_router session names are validated to [A-Za-z0-9_-] per segment (max 3 segments separated by /); after / → _ substitution the result contains only chars legal in a tmux session name (no :, no ., no /).
Behavior for Nu and Bash shells is unchanged.
The top navigation bar (Home / Router / Terminal / Admin / Docs) MUST NOT be touched.
No HTML/JS changes; the existing client attach(name) path already reuses the running PTY job, so the only path that needs fixing is the case where a tmux PTY job has exited (or never existed for this name) and the user re-creates a session under the same name.

Files to Modify/Create

crates/hero_router/src/server/terminal.rs — change the tmux ActionBuilder script string, add a small tmux_session_name helper, and add unit tests.

No changes to templates/terminal.html, static/js/terminal.js, or any RPC method shapes.

Implementation Plan

Step 1: Add a `tmux_session_name` helper

Add a private function next to encode_job_name near line 99:

/// Map a hero_router session name to a tmux-server-side session name.
///
/// hero_router session names may contain `/` (validated to max 3 segments).
/// tmux session names cannot contain `:` or `.` and `/` is reserved for
/// window/pane targets, so we substitute `/` -> `_`. All other allowed input
/// chars (`[A-Za-z0-9_-]`) are already valid tmux session-name chars.
fn tmux_session_name(name: &str) -> String {
    name.replace('/', "_")
}

It is intentionally separate from encode_job_name because the encodings serve different downstreams (hero_proc job-name space vs tmux session-name space).

Step 2: Switch the tmux launch command to `new-session -A -s <name>`

Inside create_session, replace the ShellType::Tmux arm of the match shell:

ShellType::Tmux => {
    let tmux_name = tmux_session_name(name);
    let script = format!("tmux new-session -A -s {tmux_name}");
    hero_proc_sdk::ActionBuilder::new(&job_name, script)
        .interpreter("exec")
        .tty()
        .is_process()
        .build()
}

Confirmed-safe properties:

validate_name restricts each segment to [A-Za-z0-9_-], so tmux_name contains no whitespace and cannot smuggle extra args through whitespace-split argv that the exec interpreter performs.
tmux new-session -A -s NAME per tmux(1): "If -A is given, attach to an existing session with the given name if it exists, otherwise create a new one." Exactly the required semantics.

Step 3: Update the inline comment

The existing comment that says Tmux: exec tmux directly; it manages its own session naming. becomes:

// Tmux: invoke `tmux new-session -A -s <name>` so that reconnecting to
// a hero_router session whose tmux daemon-side session already exists
// re-attaches to it (preserving layout/windows/scrollback) instead of
// creating a fresh one. The tmux server `setsid()`s itself off the PTY
// child, so it survives across hero_proc PTY job restarts.

Step 4: Add unit tests

Add a #[cfg(test)] mod tmux_naming_tests block at the end of the file:

#[cfg(test)]
mod tmux_naming_tests {
    use super::tmux_session_name;

    #[test]
    fn flat_name_passes_through_unchanged() {
        assert_eq!(tmux_session_name("work"), "work");
        assert_eq!(tmux_session_name("a-b_c"), "a-b_c");
    }

    #[test]
    fn slashes_are_replaced_with_underscores() {
        assert_eq!(tmux_session_name("work/dev"), "work_dev");
        assert_eq!(tmux_session_name("a/b/c"), "a_b_c");
    }

    #[test]
    fn name_contains_no_tmux_reserved_chars() {
        for input in ["work", "a/b", "a/b/c", "x-1_2"] {
            let n = tmux_session_name(input);
            assert!(!n.contains(':'));
            assert!(!n.contains('.'));
            assert!(!n.contains('/'));
            assert!(!n.contains(' '));
        }
    }
}

Pure, no external dependency on tmux being installed; pins the naming contract.

Step 5: Manual smoke test

cargo build -p hero_router then run the router locally with hero_proc available.
Open /terminal, create a tmux session called work. Inside, run Ctrl-b " to split horizontally and run htop in the lower pane.
Click the "X" on the pane (browser disconnect) — do NOT click "Delete session". Reload the page.
Re-attach by clicking the work entry in the sidebar. Verify the split and htop are still present.
Stop the session via the RPC terminal.delete (this kills the hero_proc PTY job; tmux daemon survives in the background).
Re-create a session named work from the New-session modal. Verify the previous tmux layout is reattached (this is the path the issue specifically targets).
Repeat with a slashed name like team/work to exercise the / -> _ mapping.

Acceptance Criteria

Reconnecting attaches to existing tmux session — covered by Step 5 manual test (delete PTY job, recreate same name, layout returns).
Window layout is preserved between sessions — covered by Step 5 (split + htop survive).
New session only created if none exists — covered by tmux new-session -A semantics; verifiable on the host with tmux ls showing exactly one entry per tmux_session_name(N).
cargo test -p hero_router tmux_naming_tests passes.
cargo clippy -p hero_router produces no new warnings.
Nu and Bash sessions still launch as before (regression check via the New-session modal).

Notes

Process-group kill risk for the tmux server. hero_proc's kill_process_tree walks parent-child relationships via sysinfo and signals each PID. When tmux is started, the FIRST tmux invocation forks a daemon that calls setsid(), becoming session leader of a NEW session and re-parenting to PID 1. From that moment, the daemon is no longer a descendant of the hero_proc PTY child, so kill_process_tree(<PTY child PID>) will NOT reach the daemon and the daemon survives — exactly what we need. This is the standard tmux contract; it should hold under hero_proc's current implementation. Tracked as a follow-up only if a future hero_proc release switches to a different kill strategy that catches the detached server.

Sanitization rationale. Hero_router's validate_name already rejects everything dangerous; the only character that can appear in a valid hero_router name but is illegal/reserved in a tmux session name is /. We map it to _. Underscores already appear freely in hero_router names, so collisions like a/b vs a_b are theoretically possible at the tmux level — but a_b and a/b already collide in hero_proc job space too (both encode to router_term_a_b__tmux after / -> __ and _ is already legal). Existing validate_name does not deduplicate these, so this is not a regression introduced by this fix; if it ever matters, a separate ticket can tighten the validator.

Backward compatibility. Sessions created BEFORE this change ran bare tmux (= a fresh anonymous session each time). Their per-launch tmux servers were daemonized but had no further reattach path; once the PTY job exited they remain idle on the host until rebooted (the tmux ls entry is named 0 by default) but are unreachable via this fix. After the change ships, every new session creates or attaches to a deterministically named tmux server-side session, and the persistence guarantee starts there. No migration is needed.

Diff size estimate. ~6–8 lines of substantive Rust changes in create_session, plus a 3-line helper, plus ~25 lines of unit tests. Well under 50 lines.

## Implementation Spec for Issue #70 ### Objective Make tmux terminal sessions in hero_router persist their layout and window state across browser disconnects and PTY-job restarts, by switching the launch command from a bare `tmux` (which always creates a fresh session) to `tmux new-session -A -s <name>` (attach if exists, else create), keyed by the user-facing session name. ### Requirements - When a tmux PTY job (re)starts for a hero_router session whose name is `N`, it MUST attach to a tmux daemon-side session named `tmux_session_name(N)` if one already exists, instead of creating a brand-new one. - A new tmux daemon-side session MUST be created only when none with that name is found. - Window/pane layout, window names, and shell history of attached panes are inherently preserved by the tmux server across client disconnects, so this single change satisfies the layout-preservation requirement. - Naming convention: `tmux_session_name(N)` = the hero_router session name with every `/` replaced by `_`. Hero_router session names are validated to `[A-Za-z0-9_-]` per segment (max 3 segments separated by `/`); after `/` → `_` substitution the result contains only chars legal in a tmux session name (no `:`, no `.`, no `/`). - Behavior for `Nu` and `Bash` shells is unchanged. - The top navigation bar (Home / Router / Terminal / Admin / Docs) MUST NOT be touched. - No HTML/JS changes; the existing client `attach(name)` path already reuses the running PTY job, so the only path that needs fixing is the case where a tmux PTY job has exited (or never existed for this name) and the user re-creates a session under the same name. ### Files to Modify/Create - `crates/hero_router/src/server/terminal.rs` — change the tmux `ActionBuilder` script string, add a small `tmux_session_name` helper, and add unit tests. No changes to `templates/terminal.html`, `static/js/terminal.js`, or any RPC method shapes. ### Implementation Plan #### Step 1: Add a `tmux_session_name` helper Add a private function next to `encode_job_name` near line 99: ```rust /// Map a hero_router session name to a tmux-server-side session name. /// /// hero_router session names may contain `/` (validated to max 3 segments). /// tmux session names cannot contain `:` or `.` and `/` is reserved for /// window/pane targets, so we substitute `/` -> `_`. All other allowed input /// chars (`[A-Za-z0-9_-]`) are already valid tmux session-name chars. fn tmux_session_name(name: &str) -> String { name.replace('/', "_") } ``` It is intentionally separate from `encode_job_name` because the encodings serve different downstreams (hero_proc job-name space vs tmux session-name space). #### Step 2: Switch the tmux launch command to `new-session -A -s <name>` Inside `create_session`, replace the `ShellType::Tmux` arm of the `match shell`: ```rust ShellType::Tmux => { let tmux_name = tmux_session_name(name); let script = format!("tmux new-session -A -s {tmux_name}"); hero_proc_sdk::ActionBuilder::new(&job_name, script) .interpreter("exec") .tty() .is_process() .build() } ``` Confirmed-safe properties: - `validate_name` restricts each segment to `[A-Za-z0-9_-]`, so `tmux_name` contains no whitespace and cannot smuggle extra args through whitespace-split argv that the `exec` interpreter performs. - `tmux new-session -A -s NAME` per `tmux(1)`: "If `-A` is given, attach to an existing session with the given name if it exists, otherwise create a new one." Exactly the required semantics. #### Step 3: Update the inline comment The existing comment that says `Tmux: exec tmux directly; it manages its own session naming.` becomes: ```rust // Tmux: invoke `tmux new-session -A -s <name>` so that reconnecting to // a hero_router session whose tmux daemon-side session already exists // re-attaches to it (preserving layout/windows/scrollback) instead of // creating a fresh one. The tmux server `setsid()`s itself off the PTY // child, so it survives across hero_proc PTY job restarts. ``` #### Step 4: Add unit tests Add a `#[cfg(test)] mod tmux_naming_tests` block at the end of the file: ```rust #[cfg(test)] mod tmux_naming_tests { use super::tmux_session_name; #[test] fn flat_name_passes_through_unchanged() { assert_eq!(tmux_session_name("work"), "work"); assert_eq!(tmux_session_name("a-b_c"), "a-b_c"); } #[test] fn slashes_are_replaced_with_underscores() { assert_eq!(tmux_session_name("work/dev"), "work_dev"); assert_eq!(tmux_session_name("a/b/c"), "a_b_c"); } #[test] fn name_contains_no_tmux_reserved_chars() { for input in ["work", "a/b", "a/b/c", "x-1_2"] { let n = tmux_session_name(input); assert!(!n.contains(':')); assert!(!n.contains('.')); assert!(!n.contains('/')); assert!(!n.contains(' ')); } } } ``` Pure, no external dependency on tmux being installed; pins the naming contract. #### Step 5: Manual smoke test 1. `cargo build -p hero_router` then run the router locally with hero_proc available. 2. Open `/terminal`, create a tmux session called `work`. Inside, run `Ctrl-b "` to split horizontally and run `htop` in the lower pane. 3. Click the "X" on the pane (browser disconnect) — do NOT click "Delete session". Reload the page. 4. Re-attach by clicking the `work` entry in the sidebar. Verify the split and `htop` are still present. 5. Stop the session via the RPC `terminal.delete` (this kills the hero_proc PTY job; tmux daemon survives in the background). 6. Re-create a session named `work` from the New-session modal. Verify the previous tmux layout is reattached (this is the path the issue specifically targets). 7. Repeat with a slashed name like `team/work` to exercise the `/` -> `_` mapping. ### Acceptance Criteria - [ ] Reconnecting attaches to existing tmux session — covered by Step 5 manual test (delete PTY job, recreate same name, layout returns). - [ ] Window layout is preserved between sessions — covered by Step 5 (split + htop survive). - [ ] New session only created if none exists — covered by `tmux new-session -A` semantics; verifiable on the host with `tmux ls` showing exactly one entry per `tmux_session_name(N)`. - [ ] `cargo test -p hero_router tmux_naming_tests` passes. - [ ] `cargo clippy -p hero_router` produces no new warnings. - [ ] Nu and Bash sessions still launch as before (regression check via the New-session modal). ### Notes **Process-group kill risk for the tmux server.** hero_proc's `kill_process_tree` walks parent-child relationships via `sysinfo` and signals each PID. When tmux is started, the FIRST `tmux` invocation forks a daemon that calls `setsid()`, becoming session leader of a NEW session and re-parenting to PID 1. From that moment, the daemon is no longer a descendant of the hero_proc PTY child, so `kill_process_tree(<PTY child PID>)` will NOT reach the daemon and the daemon survives — exactly what we need. This is the standard tmux contract; it should hold under hero_proc's current implementation. Tracked as a follow-up only if a future hero_proc release switches to a different kill strategy that catches the detached server. **Sanitization rationale.** Hero_router's `validate_name` already rejects everything dangerous; the only character that can appear in a valid hero_router name but is illegal/reserved in a tmux session name is `/`. We map it to `_`. Underscores already appear freely in hero_router names, so collisions like `a/b` vs `a_b` are theoretically possible at the tmux level — but `a_b` and `a/b` already collide in hero_proc job space too (both encode to `router_term_a_b__tmux` after `/` -> `__` and `_` is already legal). Existing `validate_name` does not deduplicate these, so this is not a regression introduced by this fix; if it ever matters, a separate ticket can tighten the validator. **Backward compatibility.** Sessions created BEFORE this change ran bare `tmux` (= a fresh anonymous session each time). Their per-launch tmux servers were daemonized but had no further reattach path; once the PTY job exited they remain idle on the host until rebooted (the `tmux ls` entry is named `0` by default) but are unreachable via this fix. After the change ships, every new session creates or attaches to a deterministically named tmux server-side session, and the persistence guarantee starts there. No migration is needed. **Diff size estimate.** ~6–8 lines of substantive Rust changes in `create_session`, plus a 3-line helper, plus ~25 lines of unit tests. Well under 50 lines.

ashraf commented

2026-05-03 12:12:29 +00:00

Member

Test Results

Build: ok (cargo build -p hero_router)
Total tests: 102
Passed: 102
Failed: 0
Ignored: 0

Breakdown:

herolib_router lib: 97 passed (94 pre-existing + 3 new in tmux_naming_tests)
hero_router bin: 5 passed (log_bridge tests)
Doc-tests: 0

New tests added by this change:

server::terminal::tmux_naming_tests::flat_name_passes_through_unchanged
server::terminal::tmux_naming_tests::slashes_are_replaced_with_underscores
server::terminal::tmux_naming_tests::name_contains_no_tmux_reserved_chars

Clippy: cargo clippy -p hero_router --no-deps produces no NEW warnings on the changed file (crates/hero_router/src/server/terminal.rs); the only clippy note in that file is the pre-existing very complex type on the attached() registry — unrelated to this change.

The added unit tests pin the tmux_session_name naming contract (no :, no ., no /, no whitespace). End-to-end persistence verification (delete PTY job, recreate same name, layout returns) requires a live tmux + hero_proc environment and is left to manual smoke testing per the spec.

## Test Results - Build: ok (`cargo build -p hero_router`) - Total tests: 102 - Passed: 102 - Failed: 0 - Ignored: 0 Breakdown: - `herolib_router` lib: 97 passed (94 pre-existing + 3 new in `tmux_naming_tests`) - `hero_router` bin: 5 passed (`log_bridge` tests) - Doc-tests: 0 New tests added by this change: - `server::terminal::tmux_naming_tests::flat_name_passes_through_unchanged` - `server::terminal::tmux_naming_tests::slashes_are_replaced_with_underscores` - `server::terminal::tmux_naming_tests::name_contains_no_tmux_reserved_chars` Clippy: `cargo clippy -p hero_router --no-deps` produces no NEW warnings on the changed file (`crates/hero_router/src/server/terminal.rs`); the only clippy note in that file is the pre-existing `very complex type` on the `attached()` registry — unrelated to this change. The added unit tests pin the `tmux_session_name` naming contract (no `:`, no `.`, no `/`, no whitespace). End-to-end persistence verification (delete PTY job, recreate same name, layout returns) requires a live tmux + hero_proc environment and is left to manual smoke testing per the spec.

ashraf commented

2026-05-03 12:13:14 +00:00

Member

Implementation Summary

Switched the tmux launch command in hero_router from a bare tmux (which always creates a new session) to tmux new-session -A -s <session_name> (attach if exists, else create). Layout / windows / scrollback are now preserved across PTY job restarts because the tmux server setsid()s itself off the PTY child, survives the client exiting, and the next launch attaches to the existing named session instead of starting fresh.

How it works

The hero_router session name (e.g. work, or team/dev) is mapped to a tmux session name via tmux_session_name(N) — / is replaced with _, all other allowed chars ([A-Za-z0-9_-]) are already valid tmux session-name chars.
On every tmux session create / re-create from the New-session modal, the spawned process runs tmux new-session -A -s <tmux_name>. tmux's -A semantics (attach if exists, otherwise create) match the issue requirement exactly.
Nu and Bash launch commands are unchanged.
The duplicate-name handling already in create_session (clean up any exited job with the same name before recreating) continues to work as before, so re-creating the same hero_router session name remains idempotent.

Files modified

crates/hero_router/src/server/terminal.rs
- Added tmux_session_name(name: &str) -> String helper near encode_job_name.
- Replaced the ShellType::Tmux arm of the match shell in create_session to build tmux new-session -A -s <tmux_name>.
- Updated the inline comment block above the match to describe the new tmux behavior.
- Added #[cfg(test)] mod tmux_naming_tests with three unit tests pinning the naming contract.

No changes to templates, JS, or any RPC method shapes.

Test results

Build: clean (cargo build -p hero_router)
Tests: 102 passed, 0 failed, 0 ignored (3 new + 99 pre-existing)
Clippy: no new warnings on the changed file.

Acceptance criteria

Reconnecting attaches to existing tmux session — guaranteed by tmux new-session -A semantics.
Window layout is preserved between sessions — inherent to tmux when re-attaching to a named server-side session.
New session only created if none exists — that is exactly what -A -s does.

Notes

Backward compatibility: tmux sessions created BEFORE this change ran bare tmux, so their server-side sessions were anonymous (named 0) and have no reattach path. After this change ships, each new hero_router session creates or attaches to a deterministically named tmux session, and the persistence guarantee starts there. No migration required.
Process lifecycle: tmux daemonizes its server via setsid() on the very first invocation, so it is no longer a descendant of the hero_proc PTY child and is not reaped by kill_process_tree. This is the standard tmux contract; persistence holds under the current hero_proc implementation.
Sanitization rationale: only / needed mapping. Existing validate_name already rejects all other characters that would be illegal or reserved in a tmux session name.

## Implementation Summary Switched the tmux launch command in hero_router from a bare `tmux` (which always creates a new session) to `tmux new-session -A -s <session_name>` (attach if exists, else create). Layout / windows / scrollback are now preserved across PTY job restarts because the tmux server `setsid()`s itself off the PTY child, survives the client exiting, and the next launch attaches to the existing named session instead of starting fresh. ### How it works - The hero_router session name (e.g. `work`, or `team/dev`) is mapped to a tmux session name via `tmux_session_name(N)` — `/` is replaced with `_`, all other allowed chars (`[A-Za-z0-9_-]`) are already valid tmux session-name chars. - On every tmux session create / re-create from the New-session modal, the spawned process runs `tmux new-session -A -s <tmux_name>`. tmux's `-A` semantics (attach if exists, otherwise create) match the issue requirement exactly. - Nu and Bash launch commands are unchanged. - The duplicate-name handling already in `create_session` (clean up any exited job with the same name before recreating) continues to work as before, so re-creating the same hero_router session name remains idempotent. ### Files modified - `crates/hero_router/src/server/terminal.rs` - Added `tmux_session_name(name: &str) -> String` helper near `encode_job_name`. - Replaced the `ShellType::Tmux` arm of the `match shell` in `create_session` to build `tmux new-session -A -s <tmux_name>`. - Updated the inline comment block above the `match` to describe the new tmux behavior. - Added `#[cfg(test)] mod tmux_naming_tests` with three unit tests pinning the naming contract. No changes to templates, JS, or any RPC method shapes. ### Test results - Build: clean (`cargo build -p hero_router`) - Tests: 102 passed, 0 failed, 0 ignored (3 new + 99 pre-existing) - Clippy: no new warnings on the changed file. ### Acceptance criteria - [x] Reconnecting attaches to existing tmux session — guaranteed by `tmux new-session -A` semantics. - [x] Window layout is preserved between sessions — inherent to tmux when re-attaching to a named server-side session. - [x] New session only created if none exists — that is exactly what `-A -s` does. ### Notes - **Backward compatibility**: tmux sessions created BEFORE this change ran bare `tmux`, so their server-side sessions were anonymous (named `0`) and have no reattach path. After this change ships, each new hero_router session creates or attaches to a deterministically named tmux session, and the persistence guarantee starts there. No migration required. - **Process lifecycle**: tmux daemonizes its server via `setsid()` on the very first invocation, so it is no longer a descendant of the hero_proc PTY child and is not reaped by `kill_process_tree`. This is the standard tmux contract; persistence holds under the current hero_proc implementation. - **Sanitization rationale**: only `/` needed mapping. Existing `validate_name` already rejects all other characters that would be illegal or reserved in a tmux session name.