Repository navigation
feat(minimald): scaffolding for the attachable session process - #375
Merged
Merged
Conversation
Contributor
|
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 ignored due to path filters (1)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds a PTY-backed SSH session host and wiring: connection handler validates shell requests (PTY + session UUID), session actor lazily manages a reusable HostHandle, Host runtime supervises PTY-backed sandboxed shells, and tests exercise attach/takeover/detach behaviors. ChangesSSH Session Host Feature
Sequence DiagramsequenceDiagram
participant SSHClient as SSH Client
participant Handler as shell_request Handler
participant Session as Session Actor
participant Host as Host Runtime
participant Sandbox as Sandboxed Shell
SSHClient->>Handler: shell_request(pty_request, session_id)
Handler->>Handler: validate PTY + UUID
Handler->>Session: lookup session by ID
Handler->>Session: send channel_success
Handler->>Session: attach(channel, config)
Session->>Host: spawn Host on first attach
Host->>Sandbox: launch /bin/bash
Host->>Host: parse vt100 from stdout
SSHClient->>Host: stdin data
Host->>Sandbox: write to PTY master
Sandbox->>Host: stdout/stderr
Host->>SSHClient: forward vt100-parsed output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
twitchyliquid64
force-pushed
the
tom/session-inner
branch
from
June 10, 2026 01:49
095d195 to
ac5f820
Compare
| match msg { | ||
| Either::Left(b) => { | ||
| let mut b = &b[..]; | ||
| while !b.is_empty() { |
Member
There was a problem hiding this comment.
Since this loop holds the select! arm until all of b is written, the read arm can't run — could that deadlock against a child that's blocked writing output? A pending-stdin buffer with its own arm might be safer.
Contributor
Author
There was a problem hiding this comment.
Yeah, its possible for a program to block on writing to stdout and not consume stdin until unblocked, which would lead to a deadlock. So some amount of buffering here would be reasonable.
twitchyliquid64
force-pushed
the
tom/session-inner
branch
from
June 10, 2026 05:54
eaf767c to
34ffff5
Compare
twitchyliquid64
marked this pull request as ready for review
June 10, 2026 18:31
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/minimald/src/session.rs (1)
214-228: ⚡ Quick winAdd timeouts to the initial echo waits as well.
These loops can block forever on a regression, which makes the test suite hang instead of failing fast. The later teardown assertions already use
tokio::time::timeout; mirroring that here would make these tests much easier to debug in CI.Also applies to: 265-279, 333-347
🤖 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 `@crates/minimald/src/sessio 8000 n.rs` around lines 214 - 228, The loop that waits for ChannelMsg::Data (using channel.wait().await) can hang indefinitely; wrap each await in tokio::time::timeout with a sensible duration (match the teardown timeouts or choose e.g. Duration::from_secs(5)) so the loop will error out fast on regressions; update the match to handle Err(elapsed) from timeout (convert to a panic or test-friendly error message including the current stdout contents) and keep the existing branch logic for Some(ChannelMsg::Data { data }) and None. Apply this change to the initial echo-wait loop around channel.wait().await and the two other analogous loops in the file (the blocks around the other channel.wait().await usages).
🤖 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 `@crates/minimald/src/connection.rs`:
- Around line 358-399: The code currently calls conn_lock.take(id) early which
consumes the pending channel; instead, inspect the pending ChannelConfig
immutably (avoid take) to validate pty, check MINIMAL_SESSION_ID_ENV and parse
SessionId and perform the session lookup (using SessionId::parse_str,
crate::MINIMAL_SESSION_ID_ENV, mngr = serv.sessions_manager(),
mngr.get_session(...)) while leaving the channel in place; only after all
validations succeed call conn_lock.take(id) and proceed to attach; if
ChannelConfig is not Clone add a small immutable accessor to read the needed
fields (pty, env_vars) so the channel isn't marked Taken on rejection paths.
In `@crates/minimald/src/session_host.rs`:
- Around line 807-819: The match arm for Ok(Err(e)) in the writable branch
currently calls todo!() which panics; instead handle PTY write failures
gracefully by treating the error as a EOF/terminal-closed event: log or record
the error, clear self.stdin_buf (so we stop trying to write), stop monitoring
writable on self.master (remove writable interest or close the master fd), and
kick the session teardown flow (invoke the session cleanup/shutdown path you
already have — e.g., call the existing session termination/cleanup method or set
the state/flag that triggers it). Locate the block handling
self.master.writable() / self.stdin_buf in session_host.rs and replace the
todo!() with these non-panicking steps so a closed PTY or exited child tears
down cleanly.
- Around line 291-293: The teardown currently unconditionally sends
ws.exit_status(0), masking real failure codes; instead, obtain the actual
child's exit status from the process/host exit path and only call
ws.exit_status(...) with that real code after the child has terminated (do not
default to 0 for detached/superseded channels). Update the teardown to await the
child/host termination result (the same place the process exit is handled), use
that exit code when calling ws.exit_status(...), and skip emitting an exit
status if no termination information is available; keep ws.eof().await and
ws.close().await as before but do not send a hardcoded 0.
- Around line 787-800: The detach chord (0x17) is handled but then still
forwarded because the branch always sets self.stdin_buf = Some((b, 0)); — change
the control flow in the Either::Left(b) branch so that after successfully
sending BindingMsg::TeardownDueToDetach and setting self.remote = None you do
not set self.stdin_buf for that buffer (i.e., return/continue or conditionally
skip assigning self.stdin_buf when b.len()==1 && b[0]==0x17), keeping the
existing tracing::warn path intact for send failures.
In `@crates/minimald/src/session.rs`:
- Around line 75-81: The attach implementation currently unwraps
ChannelConfig::pty causing a panic; update SessionHandle::attach to encode the
PTY requirement: either change the signature to accept a RequestedPty or WinSize
directly (preferred) or make attach return a Result and explicitly reject the
attach when config.pty is None. Locate the attach method and replace the unwrap
of config.pty with a null-check that returns an error or sends a rejection
message to the caller (or adjust callers to pass RequestedPty/WinSize), and
update any call sites to match the new signature/behavior; reference symbols:
attach, SessionHandle::attach, ChannelConfig::pty, RequestedPty, WinSize.
- Around line 96-106: The current mint_session_host unconditionally unwraps
Host::spawn which kills the session actor on startup failures; change
mint_session_host to await Host::spawn without unwrap, match on the Result, and
only set self.host = Some(h) on Ok(h). On Err(e) do not set self.host (leave
session alive) and surface the failure to the attach caller via the
SessionHandle (e.g., call a respond/attach-failed method on session_hnd or send
an AttachFailed message with the error), and optionally log the error; this
ensures failures in Host::spawn (PTY/sandbox/context errors) are returned as
attach failures instead of terminating the session actor.
---
Nitpick comments:
In `@crates/minimald/src/session.rs`:
- Around line 214-228: The loop that waits for ChannelMsg::Data (using
channel.wait().await) can hang indefinitely; wrap each await in
tokio::time::timeout with a sensible duration (match the teardown timeouts or
choose e.g. Duration::from_secs(5)) so the loop will error out fast on
regressions; update the match to handle Err(elapsed) from timeout (convert to a
panic or test-friendly error message including the current stdout contents) and
keep the existing branch logic for Some(ChannelMsg::Data { data }) and None.
Apply this change to the initial echo-wait loop around channel.wait().await and
the two other analogous loops in the file (the blocks around the other
channel.wait().await usages).
🪄 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: 30acc2a5-8c79-490c-a964-deafe51cdaa7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlcrates/minimald/Cargo.tomlcrates/minimald/src/connection.rscrates/minimald/src/lib.rscrates/minimald/src/main.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/test_harness.rscrates/sandbox2/src/lib.rs
Comment on lines
+358
to
+399
| let conn = self.0.clone(); | ||
| let serv = conn.0.lock().await.serv.clone(); | ||
| let mut conn_lock = conn.lock().await; | ||
| let Some((channel, config)) = conn_lock.take(id) else { | ||
| session.channel_failure(id)?; | ||
| return Ok(()); | ||
| }; | ||
| drop(conn_lock); | ||
| if config.pty.is_none() { | ||
| tracing::warn!("channel {id}: pty not requested for shell session",); | ||
| session.channel_failure(id)?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let Some(session_id_str) = config.env_vars.get(crate::MINIMAL_SESSION_ID_ENV) else { | ||
| tracing::warn!("shell request rejected on channel {id}: missing MINIMAL_SESSION_ID",); | ||
| session.channel_failure(id)?; | ||
| return Ok(()); | ||
| }; | ||
| let Ok(session_id) = SessionId::parse_str(session_id_str) else { | ||
| tracing::warn!( | ||
| value = %session_id_str, | ||
| "shell request rejected on channel {id}: not a uuid", | ||
| ); | ||
| session.channel_failure(id)?; | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let mngr = serv.sessions_manager().await; | ||
| let session_handle = match mngr.get_session(SessionKeyPredicate::Id(session_id)).await { | ||
| Ok(Some(h)) => h, | ||
| Ok(None) => { | ||
| tracing::warn!(%session_id, "shell request rejected: unknown session"); | ||
| session.channel_failure(id)?; | ||
| return Ok(()); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(%session_id, error = %e, "shell request rejected: lookup failed"); | ||
| session.channel_failure(id)?; | ||
| return Ok(()); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Delay take(id) until the shell request is fully validated.
This consumes the channel before the PTY check, MINIMAL_SESSION_ID parsing, and session lookup. On any rejection path, the channel stays in Taken, so the client cannot retry or fall back to another request on the same channel even though initiation never succeeded.
Suggested direction
Validate against the pending config first, then call take(id) only once the code is ready to attach:
- let mut conn_lock = conn.lock().await;
- let Some((channel, config)) = conn_lock.take(id) else {
+ let config = {
+ let conn_lock = conn.lock().await;
+ let Some(channel) = conn_lock.channels.get(&id) else {
+ session.channel_failure(id)?;
+ return Ok(());
+ };
+ let ChannelInner::Pending(_, config) = &channel.inner else {
+ session.channel_failure(id)?;
+ return Ok(());
+ };
+ config.clone()
+ };
+
+ let Some((channel, config)) = conn.lock().await.take(id) else {
session.channel_failure(id)?;
return Ok(());
};
- drop(conn_lock);If ChannelConfig is not Clone, an immutable accessor for just the needed fields would avoid freezing the channel early.
🤖 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 `@crates/minimald/src/connection.rs` around lines 358 - 399, The code currently
calls conn_lock.take(id) early which consumes the pending channel; instead,
inspect the pending ChannelConfig immutably (avoid take) to validate pty, check
MINIMAL_SESSION_ID_ENV and parse SessionId and perform the session lookup (using
SessionId::parse_str, crate::MINIMAL_SESSION_ID_ENV, mngr =
serv.sessions_manager(), mngr.get_session(...)) while leaving the channel in
place; only after all validations succeed call conn_lock.take(id) and proceed to
attach; if ChannelConfig is not Clone add a small immutable accessor to read the
needed fields (pty, env_vars) so the channel isn't marked Taken on rejection
paths.
Comment on lines
+291
to
+293
| let _ = ws.eof().await; | ||
| let _ = ws.exit_status(0).await; // TODO: Only report this if process terminates | ||
| let _ = ws.close().await; // needed to release the remote |
Contributor
There was a problem hiding this comment.
Report the real process exit status instead of always sending 0.
This teardown path unconditionally emits exit_status(0), so a detached/superseded channel looks like a clean shell exit and real non-zero exits are masked. The exit status needs to come from the host/process-exit path, and only when the child has actually terminated.
🤖 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 `@crates/minimald/src/session_host.rs` around lines 291 - 293, The teardown
currently unconditionally sends ws.exit_status(0), masking real failure codes;
instead, obtain the actual child's exit status from the process/host exit path
and only call ws.exit_status(...) with that real code after the child has
terminated (do not default to 0 for detached/superseded channels). Update the
teardown to await the child/host termination result (the same place the process
exit is handled), use that exit code when calling ws.exit_status(...), and skip
emitting an exit status if no termination information is available; keep
ws.eof().await and ws.close().await as before but do not send a hardcoded 0.
Comment on lines
+807
to
+819
| w = self.master.writable(), if self.stdin_buf.is_some() => { | ||
| let (buff, n) = self.stdin_buf.as_mut().unwrap(); | ||
| match w.expect("todo write handle error").try_io(|fd|fd.get_ref().write(&buff[*n..])) { | ||
| Ok(Ok(extra)) => { | ||
| if (*n+extra) == buff.len() { | ||
| self.stdin_buf = None; | ||
| } else { | ||
| *n += extra; | ||
| } | ||
| } | ||
| Ok(Err(e)) => { | ||
| todo!("handle error writing to stdin: {e}"); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle PTY write failures without panicking the host task.
Any write error here hits todo!() and aborts the background host task. A closed PTY or exited child should tear down cleanly instead of crashing the attachment/session-host path.
🤖 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 `@crates/minimald/src/session_host.rs` around lines 807 - 819, The match arm
for Ok(Err(e)) in the writable branch currently calls todo!() which panics;
instead handle PTY write failures gracefully by treating the error as a
EOF/terminal-closed event: log or record the error, clear self.stdin_buf (so we
stop trying to write), stop monitoring writable on self.master (remove writable
interest or close the master fd), and kick the session teardown flow (invoke the
session cleanup/shutdown path you already have — e.g., call the existing session
termination/cleanup method or set the state/flag that triggers it). Locate the
block handling self.master.writable() / self.stdin_buf in session_host.rs and
replace the todo!() with these non-panicking steps so a closed PTY or exited
child tears down cleanly.
Comment on lines
+96
to
+106
| async fn mint_session_host( | ||
| &mut self, | ||
| session_hnd: SessionHandle, | ||
| channel: Channel<Msg>, | ||
| sz: WinSize, | ||
| ) { | ||
| let launcher = self.session_launcher(session_hnd); | ||
| let h = Box::pin(session_host::Host::spawn(launcher, sz, Some(channel))) | ||
| .await | ||
| .unwrap(); | ||
| self.host = Some(h); |
Contributor
There was a problem hiding this comment.
Don't kill the session actor when host startup fails.
Host::spawn() can fail on PTY open, context/env construction, or sandbox spawn. Unwrapping it here turns one failed attach into a dead session actor, and subsequent SessionHandle calls then hit the "corresponding session is dead" expects. This should be surfaced as an attach failure while keeping the session alive.
🤖 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 `@crates/minimald/src/session.rs` around lines 96 - 106, The current
mint_session_host unconditionally unwraps Host::spawn which kills the session
actor on startup failures; change mint_session_host to await Host::spawn without
unwrap, match on the Result, and only set self.host = Some(h) on Ok(h). On
Err(e) do not set self.host (leave session alive) and surface the failure to the
attach caller via the SessionHandle (e.g., call a respond/attach-failed method
on session_hnd or send an AttachFailed message with the error), and optionally
log the error; this ensures failures in Host::spawn (PTY/sandbox/context errors)
are returned as attach failures instead of terminating the session actor.
norrietaylor
approved these changes
Jun 10, 2026
twitchyliquid64
force-pushed
the
tom/session-inner
branch
from
June 11, 2026 17:13
0b55e08 to
6e9dda9
Compare
twitchyliquid64
enabled auto-merge (rebase)
June 11, 2026 17:14
This was referenced Jun 11, 2026
This was referenced Jun 25, 2026
Closed
This was referenced Jul 9, 2026
This was referenced Jul 22, 2026
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
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.
The beginnings of attachable sessions.
Remaining TODOs:
Given an existing session with a
minimal.tomlin it, you should be able to spawn or reattach to the session by doing:AKA a 'request shell' ssh session - one with no command trailing.
Summary by CodeRabbit
New Features
Refactor
Chores