Repository navigation
T35 — Attach to a terminal exec in a box - #3044
Conversation
`min box attach <box> --exec <id>` reattaches this terminal to a live terminal exec in the box. It reads the box's kept events for the exec, and refuses an unknown id (exit 4), an exec that ended (exit 4), and an exec with no terminal (exit 2), in that order. clap refuses `--exec` without a box with exit 2.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Approved: every review thread resolved at @agent-137 resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The --exec refusals and their order conform to architecture.md line 491. The exec attach does not end as the design says. Line 491 and line 376 say that a stop, a removal or a takeover ends an attach with 254 after one stderr line. The --exec path returns the raw ssh code with no notice. C1 fixes this. The events fallback for live execs is an interim, and C2 labels it with #2404. C3 files the residual for the logs --exec and wait --exec verbs that the messages name. One escalation asks which code an exec's own exit gives.
Ruled at 7bbe762 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
A start event with neither pty nor mode no longer refuses a live exec as having no terminal.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 4 unresolved threads — minimal-foundry[bot] crates/minimal/src/cmd/boxes/attach.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs:1060, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs:786, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs:865. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The --exec attach finds the box and the exec and refuses in the order that line 491 gives. The compare correctly leaves an unknown PTY flag to the host, and the host refuses a non-TTY exec. Two points stay open. A box stop or removal under an exec attach must end with 254 after one line, as lines 491 and 376 say. Make that change in C1. The client derives liveness from events because live_execs is empty until #2404. Mark that as an interim with its flip in C2. The exit code for an exec that ends by itself goes to a person as an escalation.
Ruled at b1502c5 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
Notes from the agent: Revise round 1: the one review thread is valid. exec_found now refuses with NoTerminal only when the start event records pty=false or a mode that is not tty. The non-pty test asserts that a start event with neither field classifies as Terminal. cargo clippy -p minimal --all-targets is clean. |
|
/revise Arch C1 and C2 at head b1502c5, plus the maintainer's ruling on the open question. Ruling: an exec that ends by itself exits 0. architecture.md line 376: C1 (threads at attach.rs ~786 and ~1060).
C2 (threads at attach.rs ~842 and ~865). Change nothing else. |
A stop, a removal or another terminal's attach now ends the exec attach with 254 and one line, and a detach or the exec's own end with 0. Mark the event-derived liveness in exec_found as interim until #2404.
…a-terminal-exec-in-a-box
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare fixes the open conditions from b1502c53. min box attach <box> --exec <id> now ends through the same table as the bare attach. A stop or a removal is 254 with one line on stderr. A detach or the exec's own end is 0. Lines 376 and 491 say this. The doc comment on exec_found marks the event-derived liveness as an interim until #2404, with its flip condition. Nothing remains to change in this PR.
Ruled at 09fa8a1 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 6 unresolved threads — minimal-foundry[bot] crates/minimal/src/cmd/boxes/attach.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs:874, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
Round 2 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
Notes from the agent: C1: box_attach_exec_on_hosts now reads the box after the attach with box_after_attach_settled and returns an AttachEnding through the new exec_attach_ending. That function delegates to attach_ending. The ssh code 254 and 255 map as on the bare path. A gone box gives 254 with the removal line. A stopped box gives 254 with the stop line. Every other ending gives 0, per the ruling. cmd_box_attach prints ending.notice once before the exit. Settle bound: DAEMON_END_SETTLE after 254, and ENTRYPOINT_END_SETTLE after every other code. A signal-ended exec reports 1, not 0, so the box is read after every code, not only after 0. Limit: a stop that waits on a process past 500 ms after the exec died by signal will read as running, and the command exits 0. A longer bound for non-zero codes would make every exec that exits by itself with a non-zero code wait up to 12 s. C2: exec_found's doc comment marks the event-derived liveness as Interim, links #2404, and states the flip. The existing reattach test now expects exit 0 in place of the exec's code 3. just clippy-strict was not run. |
|
/revise One case is left open from C1 (thread PRRT_kwDOPdfcxM6z_kDZ), at head 09fa8a1. Arch passes. CI is green. A Fix it on the daemon side, so the client does not have to wait 12 s after every failing exec. When the box's stop (or removal) ends an attached exec, Tests:
Also make the unreachable |
A stop takes the box's host before it signals the box, and a removal marks the box first. An attached tty exec that ends after either now ends its attach channel with 254, so min box attach --exec reads the box and prints the stop line. The exec's records keep its own status.
exec_attach_ending now builds the retry command with --exec <id>. A new test pins a stopped record that lands well after the daemon's 254: the attach still ends with 254 and the stop line.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 6 unresolved threads — minimal-foundry[bot] crates/minimal/src/cmd/boxes/attach.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs:874, minimal-arch[bot] crates/minimal/src/cmd/boxes/attach.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare conforms to architecture.md line 491 and line 376. When a stop or a removal ends an attached terminal exec, the daemon now ends the attach with 254. The client then prints the stop or removal line. A lost connection gives 7, and the retry names --exec <id>. The exec's own records keep its exit code. The minimald edit is outside the task's layers, but line 491 makes the daemon the owner of the 254.
Ruled at 60273dc against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 3 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review.
Notes from the agent: Revise round 3. The daemon side: serve_exec_attach now takes the box's handle. When an attached tty exec ends, the attach channel gets 254 if the box is ending, and the exec's own status otherwise. A box is ending when a removal started (is_destroying) or when the actor no longer holds a host. A stop takes the host out of the slot before it signals any process, so the exec's death always comes after it. The exec's own exit stays in its records and in min box wait. Files outside the layers: crates/minimald/src/exec/bridge.rs (accepted by the maintainer), crates/minimald/src/exec.rs (one call site passes the handle), crates/minimald/src/box_actor/handle.rs (a new BoxHandle::has_host reader, plus two cfg(test) helpers that empty the host slot and mark a removal), and crates/minimald/src/exec/tests.rs (the daemon-side test). One side effect: the end of the entrypoint also takes the host, so an exec that the box's own exit ends now gives 254. The client then reads the box as exited and ends with 0 and the existing "Detached: exited with code N." line, where before it ended with 0 and no line. The SSH_CONNECTION_FAILED arm in exec_attach_ending now names |
There was a problem hiding this comment.
Reviewed at 60273dc against #2911 (BCLI-068 --exec). min box attach --exec resolves a live PTY exec in the named box and ends through #3018's ending table. 5a4c3f5 makes the daemon end an exec attach with 254 when the box's end (stop, removal, entrypoint end) ended the exec, keeping the exec's own status in its records, so a stop of a box whose entrypoint survives SIGTERM still reads 'Detached: was stopped.' (daemon and late-record client tests). The exec's own end exits 0 per the maintainer's ruling (architecture.md line 376). 9c83fb5 names --exec in the lost-connection retry.
|
/review |
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @agent-137 closed the last one. The review of 60273dc9fd99 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
T35 — Attach to a terminal exec in a box
slice: S3
kind: task
covers: BCLI-068
size: M
est: 100 code, 300 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: for an exec id the box never ran,
min box attach <box> --exec <id>prints " has no exec ." and exits 4; for an exec that ended, it prints "Exec has ended. Read its output with min box logs --exec , or its exit code with min box wait --exec ." and exits 4, pinned by box_attach_exec_ended_exit4_names_logs_and_waitobservability: the command 8000 's existing tracing span records the verb, the box id it resolved and the outcome
layers:
- client:
- crates/minimal/src/cli.rs: the verb's arguments and help in the one command model
- crates/minimal/src/cmd/boxes/attach.rs:
--exec <id>reattaches a live terminal exec, with the unknown, ended, no-terminal and no-box refusals in that order- crates/minimal/src/main.rs:
failure_exit_codemaps this verb's refusals to their exit codes- crates/minimal/src/tests.rs: the in-process minimald test harness gains this layer's tests
test: cargo nextest run -p minimal box_attach_exec_reattaches_live_pty_exec
test: cargo nextest run -p minimal box_attach_exec_unknown_id_exit4_names_id
test: cargo nextest run -p minimal box_attach_exec_non_pty_exit2_names_logs
test: cargo nextest run -p minimal box_attach_exec_ended_exit4_names_logs_and_wait
test: cargo nextest run -p minimal box_attach_exec_without_box_exit2
produces: none
consumes:
- crates/minimal/src/cmd/boxes/address.rs
proof:
min box attach <box> --exec <id>reattaches a terminal exec, and refuses an exec that ended or has no terminal.depends on:
- blocked by #2904
- blocked by #2895
Proof
A person can run
min box attach <box> --exec <id>to reattach their terminal to a live terminal exec in the box. The command refuses an exec id the box never ran (exit 4), an exec that ended (exit 4, naming min box logs and min box wait with --exec), an exec with no terminal (exit 2, naming min box logs --exec), and --exec with no box (exit 2).Notes from the agent: 1. The spec says to resolve the exec id against the live exec ids from BOX-159. The daemon's ShowBox still returns an empty live_execs list on this branch (box_manager.rs, issue #2404). So the client reads the box's kept events (exec_started, exec_exited and their pty field), and also counts any id in live_execs as live. An exec of a box that does not run counts as ended. The two blockers (#2904, #2895) are not visible in git log.
2. The harness daemon never starts a sandbox, so it runs no exec. The tests write the exec events that the daemon writes into the box's events file. Then they drive box_attach_exec_on_hosts with a fake attach. The real ssh -tt attach path (attach_exec_by_id) is not exercised by these tests. It mirrors the attach_exec_terminal function in exec.rs, which is outside the layers, so attach.rs has its own copy.
3. main.rs needed no change: failure_exit_code already maps every BoxAttachRefusal through exit_code(), and the new variants live on that type.
4. Printed text: the unknown-id and ended-exec messages copy the task's exact wording. The ended message and the no-terminal message name
min box logs <box> --exec <id>andmin box wait <box> --exec <id>, as the spec requires. These flags do not exist on this branch yet, so the instruction is not runnable as written until the tasks for BCLI-056 and BCLI-057 land. The no-terminal wording is mine: "Exec in has no terminal to attach to. Runmin box logs <box> --exec <id>to see its output."5. A completed attach exits with the exec's own code, as
min box exec -tdoes. ssh's 255 maps to the existing exit-7…[truncated]Tests
cargo nextest run -p minimal box_attach_exec_reattaches_live_pty_execcargo nextest run -p minimal box_attach_exec_unknown_id_exit4_names_idcargo nextest run -p minimal box_attach_exec_non_pty_exit2_names_logscargo nextest run -p minimal box_attach_exec_ended_exit4_names_logs_and_waitcargo nextest run -p minimal box_attach_exec_without_box_exit2Verify
min task run agent-verifyexited 100 under rustc 1.99.0, which the repository does not pin (1.99.1 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Closes #2911