Repository navigation
feat(minimald): T44 — Detach from an exec attach with the detach keys - #3341
minimal-foundry[bot] wants to merge 7 commits into
Conversation
serve_exec_attach now runs the leader and detach state machine of the box's own terminal, with the keys the channel negotiated. A detach ends the channel with status 0 and leaves the exec running (BCLI-068).
min box attach <box> --exec <id> now sends the session keys from the config, as the box attach does, so the daemon detaches on them (BCLI-068).
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: no findings; approval withheld, 2 threads unresolved at APPROVE withheld: 2 unresolved threads — minimal-arch[bot] crates/minimald/src/exec.rs:501, minimal-foundry[bot] crates/minimald/src/exec/bridge.rs:2290. Resolving the last thread releases the approval on this head. Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required. Clobber: none seen. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The min box attach --exec path conforms. The daemon detaches with status 0 and keeps the exec running, as architecture.md lines 492 and 498 say. The first attach of min box exec -t still sends no keys. The daemon now gives it the default chord, so a remapped leader or detach key does not apply there. Line 498 says a -t exec re-attaches like a PTY box, and the PTY box uses the configured keys. The PR body names this gap but links no issue. Make the edit in C1, or file the issue and name it as a residual.
Ruled at b1370c2 against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
| spawn(serve_exec_attach(channel, session_handle, session_id, exec_id).instrument(span)); | ||
| // The keys the client negotiated, as a box attach reads them | ||
| // (BCLI-068). A client that sent none gets the default keys. | ||
| let keys = boxes::keys::SessionKeys::from_env(&config.env_vars).validated_or_default(); |
There was a problem hiding this comment.
C1 · Here a channel without keys gets the default chord. The first attach of min box exec -t sends no keys (attach_exec_terminal in crates/minimal/src/cmd/boxes/exec.rs). So a remapped leader or detach key does not apply there. Send the configured keys from that client path, as C1 says.
There was a problem hiding this comment.
Addressed in 9a4287f2247c: attach_exec_terminal in min box exec -t now builds its ssh through exec_attach_ssh, so it sends the session keys from --config-dir like min box attach --exec.
| if detach { | ||
| out.write_all(detached_notice(&entry.id).as_bytes()).await?; | ||
| out.flush().await?; | ||
| return Ok(AttachEnd::Detached); |
There was a problem hiding this comment.
MEDIUM · correctness · confidence 0.60 · in relay_terminal_keyed
The detach path awaits out.write_all and out.flush for the notice before it returns Detached. If the client stopped reading, the SSH window is full and these awaits never finish, so the detach chord does nothing. The box terminal in box_runtime.rs (KeyAction::Detach) bounds this with send_timeout and sheds the binding, because the detach chord is how a user leaves a stalled attach. Bound the notice write with a timeout, or return Detached first and ignore a write error. The bell write in EnterCommandMode has the same unbounded await.
What the reviewer checked
out.write_all(detached_notice(&entry.id).as_bytes()).await?; out.flush().await?; return Ok(AttachEnd::Detached);
Hand this to an agent
Act on a code-review finding.
File: crates/minimald/src/exec/bridge.rs
Lines: 2265-2268
Category: correctness
Severity: medium (reviewer confidence 0.60)
Finding:
The detach path awaits `out.write_all` and `out.flush` for the notice before it returns `Detached`. If the client stopped reading, the SSH window is full and these awaits never finish, so the detach chord does nothing. The box terminal in `box_runtime.rs` (KeyAction::Detach) bounds this with `send_timeout` and sheds the binding, because the detach chord is how a user leaves a stalled attach. Bound the notice write with a timeout, or return `Detached` first and ignore a write error. The bell write in `EnterCommandMode` has the same unbounded await.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
Addressed in 9a4287f2247c: The bell and the detach notice now go through write_bounded (EXIT_NOTICE_BOUND, errors ignored). A new test pins a detach with status 0 for a client that does not read.
…m-an-exec-attach-with-the-detach-keys
A client that does not read held the detach on the notice write. The writes now wait at most EXIT_NOTICE_BOUND, and the detach goes on.
The first terminal attach of min box exec -t sent no keys, so a remapped leader or detach key did not apply. It now builds its ssh command through exec_attach_ssh, as min box attach --exec does.
|
/revise Head 1. C1: 2. The detach can't stall behind a full window (thread PRRT_kwDOPdfcxM623fwj). Reply on both threads with the commit. Run the whole |
|
That Nothing to type again: the command gets a rocket when its round starts, or a refusal that names the reason. No round was counted for it yet. |
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
Ctrl-] then d ends Notes from the agent: This round touched crates/minimal/src/cmd/boxes/exec.rs, which is outside the layers. The task's attach.rs note asks that |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change conforms to the design. A -t exec now detaches with the detach keys, ends the attach with 0, and runs on, as architecture.md line 498 and line 492 require. The two commits after the earlier head close its gaps. The detach notice cannot hold the detach, and min box exec -t sends the configured keys. The PR body still says min box exec -t sends no keys. Update that paragraph before the PR leaves draft.
Ruled at 9a4287f against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
There was a problem hiding this comment.
APPROVE withheld: 2 unresolved threads — minimal-arch[bot] crates/minimald/src/exec.rs:501, minimal-foundry[bot] crates/minimald/src/exec/bridge.rs:2290.
minimal-review — advisory review
Verdict COMMENT — No findings.
Panel: 2 of 3 personas ran (correctness, security); did not complete: supply-chain.
T44 — Detach from an exec attach with the detach keys
slice: S4
kind: task
covers: BCLI-068
size: M
est: 140 code, 160 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: a detach from an exec attach exits 0, pinned by exec_attach_detach_key_returns_0_exec_keeps_running
observability: none new
layers:
- client:
- crates/minimal/src/cmd/boxes/attach.rs: the exec attach and
min box exec -t's first attach send the session keys, as the box attach does- crates/minimal-client/src/attach.rs:
attach_commandsends the session keys on an ExecAttach channel- crates/minimal/src/tests.rs: the in-process minimald test harness gains this layer's tests
test: cargo nextest run -p minimal exec_attach_detach_key_returns_0_exec_keeps_running
- daemon:
- crates/minimald/src/exec/bridge.rs:
serve_exec_attachruns the same leader/detach state machine as the box PTY attach. A detach ends the channel as a detach, with exit 0, and leaves the exec running.test: cargo nextest run -p minimald serve_exec_attach_detach_key_ends_channel_exec_runs
produces: none
consumes: none
proof: Ctrl-] then d ends
min box attach <box> --exec <id>with exit 0, the exec keeps running, and a second attach reaches it.Proof
Ctrl-] then d (or the configured leader and detach keys) ends
min box attach <box> --exec <id>with exit 0. The terminal showsDetached from exec <id>. It keeps running., the exec keeps running,min box showstill lists it, and a secondmin box attach <box> --exec <id>reaches it.Notes from the agent: Out of scope: crates/minimald/src/exec.rs changed in one call site. serve_exec_attach needs the keys the channel negotiated, and only the dispatcher in exec.rs holds the channel's env vars. Without this change the daemon cannot honour a remapped leader or detach key.
Not done:
min box exec -t's first attach is in crates/minimal/src/cmd/boxes/exec.rs (attach_exec_terminal), not in attach.rs, and that path has no config_dir. It still sends no keys. The daemon now gives a channel without keys the default keys, so Ctrl-] d detaches there too, but a remapped chord in config.toml does not apply tomin box exec -t. A follow-up can thread global.config_dir into TtyAttach::Terminal and call exec_attach_ssh.minimal-client/src/attach.rs: attach_command already sent the keys when given Some. Onl 10000 y its doc comments changed, to say an ExecAttach channel sends them.
New printed text, not fixed by the task: the daemon writes
Detached from exec <id>. It keeps running.on the terminal at a detach. The box attach writesDetaching from session.in the same place. The exec detach does not write terminal unwind codes, because the daemon does not track the exec terminal's modes.The minimal test cannot drive a real detach through the in-process harness, because TestServer::list_running_exec registers an exec with no terminal. It checks the keys the attach's ssh sends (default and configured), the 0 ending, the live listing and a second attach. The minimald test drives the detach through attach_terminal_keyed on a live terminal.
Tests
cargo nextest run -p minimal exec_attach_detach_key_returns_0_exec_keeps_runningcargo nextest run -p minimald serve_exec_attach_detach_key_ends_channel_exec_runsVerify
min task run agent-verifytimed out after 1800s, so this PR is a draft.Out of scope
These files are outside the task's
layers:, so this PR is a draft:crates/minimald/src/exec.rsCloses #3331