feat(minimal,minimald): confirm single-session destroy with the workspace delta - #1148
Conversation
…pace delta min session destroy deleted unsaved in-session work without a word. The single-session path now lists the files changed since activation (a new oneshot RPC serving the host's delta baseline) and confirms with default No; headless runs refuse without --force, which is unhooked from --all. Delta failures degrade to a plain confirm — destroy never blocks on the listing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KyZLpkRf9G4A2hUDgDvn5f
📝 WalkthroughWalkthroughThe PR adds a ChangesSession destruction confirmation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…w dirty gate The headless destroy of the --keep session now trips the gate this PR introduces — which is the feature working. The teardown asserts the refusal (and that it names --force) before destroying with --force. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KyZLpkRf9G4A2hUDgDvn5f
|
The |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/minimal/src/lib.rs (1)
2390-2411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared row-cap/formatting into one place.
print_destroy_deltare-implements the header text, row cap, and "... and N more" line that the shell-exit prompt leads with — an unavailable delta renders the plain prompt so the exit path never blocks on change detection already renders incrates/minimald/src/session_host.rs'sshell_exit_prompt. The code even documents the coupling as a comment rather than a shared source of truth: "Cap on rows printed, matching the shell-exit prompt's."Two independently maintained copies of the same wording and magic number (
10) will drift silently if one side changes. Move the row cap (and ideally the row-rendering logic) intominimald-rpc, which bothcrates/minimalandcrates/minimaldalready depend on, so both call sites share one constant/function.♻️ Suggested direction
-fn print_destroy_delta(changed: &Option<Vec<String>>) { - /// Cap on rows printed, matching the shell-exit prompt's. - const ROWS_SHOWN: usize = 10; +fn print_destroy_delta(changed: &Option<Vec<String>>) { + use minimald_rpc::DELTA_ROWS_SHOWN as ROWS_SHOWN; match changed {(Define
pub const DELTA_ROWS_SHOWN: usize = 10;once inminimald-rpc, and havesession_host.rsreference the same constant instead of its own local one.)🤖 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/minimal/src/lib.rs` around lines 2390 - 2411, Extract the shared delta row limit into minimald-rpc as a public DELTA_ROWS_SHOWN constant, then update print_destroy_delta and shell_exit_prompt in session_host.rs to use it instead of local magic numbers or duplicated limits. Preserve the existing header, row, and overflow wording while centralizing the shared cap.
🤖 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/minimal/src/lib.rs`:
- Around line 2368-2388: The session_delta timeout path must clean up the
timed-out SSH channel before the shared client is reused by destroy_session.
Update session_delta or its caller around client.oneshot_rpc to explicitly close
the channel during cancellation, or recreate the SSH client before
destroy_session, while preserving the existing best-effort None behavior for RPC
failures and timeouts.
---
Nitpick comments:
In `@crates/minimal/src/lib.rs`:
- Around line 2390-2411: Extract the shared delta row limit into minimald-rpc as
a public DELTA_ROWS_SHOWN constant, then update print_destroy_delta and
shell_exit_prompt in session_host.rs to use it instead of local magic numbers or
duplicated limits. Preserve the existing header, row, and overflow wording while
centralizing the shared cap.
🪄 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: 9284235a-0cb6-47ef-ad1c-42ab3f1129ab
📒 Files selected for processing (5)
crates/minimal/src/lib.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rs
| /// Best-effort fetch of the files changed since activation, for the destroy | ||
| /// confirm. Any failure — RPC error, timeout, a daemon predating the RPC — | ||
| /// reads as `None`, and the confirm renders without the listing. | ||
| async fn session_delta( | ||
| client: &mut client::Client, | ||
| id: sessions::SessionId, | ||
| ) -> Option<Vec<String>> { | ||
| /// Client-side ceiling on the delta fetch. The daemon bounds its | ||
| /// workspace walk at 5 s; this sits just above so a slow-but-healthy | ||
| /// walk still answers while a wedged daemon cannot stall the confirm. | ||
| const SESSION_DELTA_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(6); | ||
| use minimald_rpc::{SessionDelta, SessionDeltaRequest}; | ||
| tokio::time::timeout( | ||
| SESSION_DELTA_TIMEOUT, | ||
| client.oneshot_rpc::<SessionDelta>(SessionDeltaRequest::Id(id)), | ||
| ) | ||
| .await | ||
| .ok()? | ||
| .ok()? | ||
| .changed | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does russh guarantee that dropping an in-flight client channel operation (mid channel_open_session or mid read) leaves the client::Handle safe to reuse for further channels?
💡 Result:
Yes, dropping an in-flight client channel operation in the russh library does not invalidate the client::Handle, and the handle remains safe to reuse for further channels [1][2]. The russh::client::Handle is designed as an interface to send messages to the SSH session asynchronously [1][2]. It uses an internal MPSC (multi-producer, single-consumer) channel (via the sender field) to communicate with the session's event loop, which manages multiplexing and channel state [3][4][2]. Because the Handle acts as a decoupled sender, dropping a future representing an in-flight operation (such as channel_open_session or a read/write operation) only cancels that specific pending request; it does not shut down the session or invalidate the underlying Handle [1][3]. Since the event loop and session state are managed independently of the specific futures created by the Handle, you can continue to use the same Handle to open new channels or send further messages after an operation is dropped [1][2]. If an operation is cancelled, the session will continue to process other messages normally, provided the SSH connection itself remains healthy [4][2].
Citations:
- 1: https://docs.rs/russh/latest/russh/client/struct.Handle.html
- 2: https://docs.rs/russh/latest/russh/client/index.html
- 3: https://github.com/Eugeny/russh/blob/main/russh/src/client/mod.rs
- 4: https://docs.rs/russh/latest/russh/client/struct.Session.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- russh dependency declarations ---'
rg -n -C 3 'russh' --glob 'Cargo.toml' --glob 'Cargo.lock' .
printf '%s\n' '--- Client and oneshot_rpc definitions/usages ---'
rg -n -C 8 'struct Client|fn oneshot_rpc|channel_open_session|destroy_session' crates
printf '%s\n' '--- vendored or cached russh sources ---'
find . -path '*/russh*' -type f -maxdepth 6 2>/dev/null | head -80
find "${CARGO_HOME:-/nonexistent}" -path '*russh*' -type f 2>/dev/null | head -80Repository: gominimal/minimal
Length of output: 47483
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- minimal Client::oneshot_rpc ---'
sed -n '220,305p' crates/minimal/src/client.rs
printf '%s\n' '--- russh 0.62.4 source locations from crates.io metadata ---'
curl --fail --silent --show-error https://crates.io/api/v1/crates/russh/0.62.4 \
| jq -r '.version.downloads, .crate, .repository'
printf '%s\n' '--- russh 0.62.4 source archive inspection ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl --fail --silent --show-error -L https://static.crates.io/crates/russh/russh-0.62.4.crate \
-o "$tmpdir/russh.crate"
tar -xzf "$tmpdir/russh.crate" -C "$tmpdir"
rg -n -C 8 'pub struct Handle|channel_open_session|struct Channel|impl.*Drop|channel_open' \
"$tmpdir/russh-0.62.4/src"Repository: gominimal/minimal
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl --fail --silent --show-error -L \
https://static.crates.io/crates/russh/russh-0.62.4.crate \
-o "$tmpdir/russh.crate"
tar -xzf "$tmpdir/russh.crate" -C "$tmpdir"
src="$tmpdir/russh-0.62.4/src"
printf '%s\n' '--- client Handle and channel_open_session ---'
rg -n -C 20 'pub struct Handle|pub async fn channel_open_session|wait_channel_confirmation' \
"$src/client" "$src/server"
printf '%s\n' '--- channel ownership and drop behavior ---'
rg -n -C 12 'ChannelCloseOnDrop|impl.*Drop|channel_close|ChannelMsg::Close|close\(' \
"$src/channels" "$src/client"
printf '%s\n' '--- pending channel-open state and reply handling ---'
rg -n -C 16 'PendingChannelOpen|ChannelOpenReply|channel_ref|oneshot' \
"$src/client" "$src/channels"Repository: gominimal/minimal
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl --fail --silent --show-error -L \
https://static.crates.io/crates/russh/russh-0.62.4.crate \
-o "$tmpdir/russh.crate"
tar -xzf "$tmpdir/russh.crate" -C "$tmpdir"
src="$tmpdir/russh-0.62.4/src"
printf '%s\n' '--- client channel-open implementation ---'
sed -n '270,310p' "$src/client/mod.rs"
sed -n '580,625p' "$src/client/mod.rs"
sed -n '680,705p' "$src/client/mod.rs"
printf '%s\n' '--- client message definitions and event-loop handling ---'
rg -n -C 12 'enum Msg|ChannelOpenSession|ChannelOpenReply|channels:|HashMap.*Channel|channels\.insert|channels\.remove' \
"$src/client" "$src"
printf '%s\n' '--- channel reference and channel-close-on-drop implementation ---'
rg -n -C 20 'struct ChannelRef|impl.*ChannelRef|struct ChannelCloseOnDrop|impl.*ChannelCloseOnDrop|Drop for Channel' \
"$src/channels" "$src"Repository: gominimal/minimal
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl --fail --silent --show-error -L \
https://static.crates.io/crates/russh/russh-0.62.4.crate \
-o "$tmpdir/russh.crate"
tar -xzf "$tmpdir/russh.crate" -C "$tmpdir"
src="$tmpdir/russh-0.62.4/src"
printf '%s\n' '--- ChannelRef definition ---'
rg -n 'struct ChannelRef|impl.*ChannelRef|ChannelRef' "$src/channels/mod.rs"
sed -n '80,145p' "$src/channels/mod.rs"
printf '%s\n' '--- confirmation and failure handling ---'
sed -n '330,455p' "$src/client/encrypted.rs"
printf '%s\n' '--- channel-close handling ---'
sed -n '390,430p' "$src/client/encrypted.rs"
sed -n '1335,1360p' "$src/client/mod.rs"
printf '%s\n' '--- low-level channel allocation ---'
rg -n -C 10 'fn channel_open_generic|fn channel_open_session' \
"$src/client/session.rs" "$src/session.rs"Repository: gominimal/minimal
Length of output: 13697
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl --fail --silent --show-error -L \
https://static.crates.io/crates/russh/russh-0.62.4.crate \
-o "$tmpdir/russh.crate"
tar -xzf "$tmpdir/russh.crate" -C "$tmpdir"
python3 - "$tmpdir/russh-0.62.4/src" <<'PY'
import pathlib, re, sys
root = pathlib.Path(sys.argv[1])
client_mod = (root / "client/mod.rs").read_text()
client_encrypted = (root / "client/encrypted.rs").read_text()
channels_mod = (root / "channels/mod.rs").read_text()
channel_ref = (root / "channels/channel_ref.rs").read_text()
def section(text, start, end):
m = re.search(start + r".*?" + end, text, re.S)
if not m:
raise SystemExit(f"missing section: {start}")
return m.group(0)
open_fn = section(
client_mod,
r"pub async fn channel_open_session",
r"\n }\n\n /// Request an X11 channel",
)
handle_fn = section(
client_mod,
r"fn handle_msg",
r"\n fn finalize_server_channel_open_reply",
)
confirmation = section(
client_encrypted,
r"Some\(\(&msg::CHANNEL_OPEN_CONFIRMATION",
r"\n Some\(\(&msg::CHANNEL_CLOSE",
)
print("channel_open_session sends ChannelOpenSession, then awaits confirmation:")
print(" ", " ".join(open_fn.split()))
print("handle_msg inserts the ChannelRef into self.channels:")
print(" ", " ".join(handle_fn[handle_fn.find("Msg::ChannelOpenSession"):handle_fn.find("Msg::ChannelOpenX11")].split()))
print("confirmation sends ChannelMsg::Open but does not remove the channel:")
print(" ", " ".join(confirmation.split()))
print("Channel has an explicit Drop implementation:",
bool(re.search(r"impl(?:<[^>]*>)?\s+Drop\s+for\s+Channel", channels_mod)))
print("ChannelRef has an explicit Drop implementation:",
bool(re.search(r"impl(?:<[^>]*>)?\s+Drop\s+for\s+ChannelRef", channel_ref)))
print("channel close removes the channel:",
"self.channels.remove(&channel_num)" in client_encrypted)
PYRepository: gominimal/minimal
Length of output: 2020
Clean up the timed-out SSH channel before reusing client.
russh 0.62.4 keeps the shared Handle reusable, but dropping oneshot_rpc does not close the channel. A timeout can leave an orphaned channel and server RPC running. Send CHANNEL_CLOSE during cancellation or recreate the SSH client before destroy_session.
🤖 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/minimal/src/lib.rs` around lines 2368 - 2388, The session_delta
timeout path must clean up the timed-out SSH channel before the shared client is
reused by destroy_session. Update session_delta or its caller around
client.oneshot_rpc to explicitly close the channel during cancellation, or
recreate the SSH client before destroy_session, while preserving the existing
best-effort None behavior for RPC failures and timeouts.
| .context("a session or --all is required")?; | ||
| let record = resolve_session(&mut client, session).await?; | ||
|
|
||
| match destroy_gate(args.force, global.no_input, std::io::stdin().is_terminal())? { |
There was a problem hiding this comment.
No need for such verbosity, fold all those conditionals into one match here with different arms
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum SessionDeltaRequest { | ||
| Name(String), |
There was a problem hiding this comment.
For listing sessions and fetching sessions it made sense for there to be a query path based on name or id.
In this case this is an internal RPC where you always know the session ID, so just take an ID and lookup via ID.
…tivation delta The activation-delta gate over-claimed: committed-and-pushed work still listed as "changed". The SessionDelta RPC now reports a precision ladder — VCS-exact (uncommitted files via git status --porcelain, unpushed commits via rev-list --branches --not --remotes) when the workspace is a real git repository, the activation delta otherwise, with an honest "may include committed work" header — and the client gates dirty-only: proven-clean sessions destroy without a word (even headless), dirty sessions confirm with the listing, and unknowable state (stopped session, RPC failure) confirms without one, refusing headless without --force in both non-clean arms. Git failures degrade down the ladder, never error, bounded by the walk timeout. Review follow-ups folded in: the request is by id only (callers have already resolved the record), the gate is one match over (force, at-risk, interactivity), and the pre-existing headless destroy tests (cli.rs, session-e2e) now assert the refusal-then---force path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KyZLpkRf9G4A2hUDgDvn5f
|
Revised per review — thanks @twitchyliquid64 for the catch that the activation-delta gate over-claimed: committed-and-pushed work still listed as "changed". The gate now fires only for genuinely at-risk work:
Verified via cross (aarch64-musl): |
# Conflicts: # crates/minimald/src/session_host.rs
min session destroydeletes unsaved in-session work without a word; it now lists the files changed since activation and confirms with default No — and headless runs refuse without--force, so EOF can never read as consent.--allprecedent;--forceis unhooked from--alland skips the confirm.--allunchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01KyZLpkRf9G4A2hUDgDvn5f
Note
Add confirmation prompt with workspace delta to single-session destroy
min session destroynow prompts for confirmation before destroying, showing changed files since activation when available (up to 10 rows).destroy_gatefunction decides whether to prompt or proceed:--forceskips the prompt; headless runs (no TTY or--no-input) without--forcereturn an error instructing the user to pass--force.SessionDeltaRPC fetches changed files from the daemon with a 6-second timeout; timeouts and errors silently map toNone, so the prompt still appears without the file listing.--forceflag is no longer restricted to--alland now works for single-session destroys as well.--forcenow fails with an explicit error instead of proceeding.Changes since #1148 opened
SessionDeltaRequestenum with struct and introduced taggedSessionDeltaResponseenum inminimald-rpc[87280ea]minimalclient [87280ea]Macroscope summarized c10c935.
Summary by CodeRabbit
New Features
--forcesupport for destroying individual sessions or all sessions.Bug Fixes
--forceis provided.