Repository navigation
T15 — Run a command in a running box - #3009
Conversation
Runs a command in a running box attached, with a terminal (-t) or detached, under an optional --timeout, and exits with the command's code. The not-running and exec-off refusals exit 2 and 5, and an exec that its own timeout ended exits 124 with the timeout code.
…nels The relay runs over an exec-channel trait, so scripted channels prove the run, -t, --detach and --timeout paths. The harness daemon cannot spawn a command from this crate, and proves the refusals.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none seen in the hunks read. |
…and-in-a-running-box
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
Two behaviours deviate from the design. First, min box exec -t without a terminal starts a PTY exec over pipes. Line 497 and principle 3 say it must exit 2 before anything starts. C1 fixes this. Second, the exit-2 refusal says " is not running." and drops the state. Line 363 requires the state, and the daemon already sends it. C2 fixes this. This PR also closes #2905, but scripts/session-e2e.sh still sends exec start over ssh until #2905 closes. C3 moves that case to min box exec and adds the real end-to-end leg. Placement, the stdout and stderr split, --detach, --timeout and the exit-5 refusals conform.
Outside the diff
scripts/session-e2e.sh:1656: C3 · This interim moves tomin box exec -t|--detachwhen #2905 closes, and this PR closes #2905. Start the terminal exec and the detached exec inproof_exec_reattachwithmin box exec. Add one attached exec that checks its exit code. Change the comment so that it names only #2913.
Ruled at 82f7955 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
The terminal attach reads stdin itself. A pump thread started before it kept a blocking read and took the first keystroke.
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
min box exec -t without a terminal still runs the command over pipes. Line 497 of architecture.md says -t requires a client TTY and exits 2 otherwise. Line 363 gives exit 2 to a PTY-requiring command that runs without a TTY. C1 adds the refusal and a test for it. C2 files the end-to-end work that the PR body leaves open. The merge of main and the stdin pump fix are correct.
Ruled at c1c3d78 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: The five test lines ran as one nextest invocation with a filter expression that names the five tests (6 tests passed, 877 skipped). That avoided five separate test builds. The fix is in cmd_box_exec only: a terminal on stdin is now never pumped, in attached mode or in -t mode. |
|
/revise Arch conditions C1 and C2 are open at head c1c3d78. Fix both. C1 (threads zzVwm, zzVws, zzlXu, zzlXy, zzlX5). Refuse
C2 (threads zzVwz, zzVw2). The not-running refusal must name the box's state (BOX-149, BCLI-053, the architecture's exit-code table). Today cli.rs:599 and exec.rs:513 replace the daemon's "box web is stopped. Only a running box can run a command." with "web is not running.".
Also:
Change nothing else. |
…and-in-a-running-box
-t with a stdin that is not a terminal now exits 2 before the daemon starts or the box resolves, and a tty exec always attaches the terminal. The not-running refusal names the state the host reports. The channel open error says only that the channel did not open.
…and-in-a-running-box # Conflicts: # crates/minimal/src/tests.rs
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare fixes the open items. min box exec -t now refuses a stdin that is not a terminal with exit 2, before the box resolves (line 497, principle 3). The not-running refusal now names the state (line 363). BCLI-053 conforms. One condition stays open. scripts/session-e2e.sh line 1656 names #2905 as the flip condition for its interim exec route, and this PR closes #2905 without moving that case. No test drives the real-terminal -t path. File the issue and repoint the comment, as C1 says.
Ruled at d800f26 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 2 of 10 on a /revise note, pushed as Resolved conflicts in:
Verify Review threads the coder judged:
Notes from the agent: Revise round 2. C1: cmd_box_exec checks the terminal before ensure_daemon, and box_exec_on_hosts checks it again before the box resolves. The refusal is BoxExecRefusal::NoTerminal (exit 2, outcome "no terminal" on the tracing event). A tty exec now always takes TtyAttach::Terminal. TtyAttach::Pipes is #[cfg(test)] and only the scripted seam uses it. C2: NotRunning carries the state word, read from the daemon text "box is . Only a running box can run a command.". When the state word cannot be read, the daemon text goes to stderr unchanged with exit 2. The plan's diagnostics line pins " is not running.". The spec and the architecture require the state, so the pinned text is now "web is stopped. Only a running box can run a command.". HostChannels::open now says "Could not open a channel to to run the command.", because the error can be a refusal or a connection failure. The module doc of exec.rs now says |
|
/revise Three small fixes at head d800f26. The C1/C2 work is correct, so leave it as it is.
Change nothing else. |
Name the box in the min box exec -t refusal, one fact per sentence, and point the e2e exec case at #3012.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare fixes the -t refusal text and the stdin pump abort. The refusal still exits 2, as architecture.md line 497 requires. BCLI-053 conforms. One leg stays open: no test drives min box exec -t on a real terminal. C1 keeps that leg open under #3012. Add the e2e case there before you close that issue.
Ruled at da9de7e against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 13 unresolved threads — minimal-foundry[bot] crates/minimal/src/cmd/boxes/exec.rs:365, minimal-arch[bot] crates/minimal/src/cmd/boxes/exec.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/exec.rs:169, minimal-arch[bot] crates/minimal/src/cli.rs, minimal-arch[bot] crates/minimal/src/cmd/boxes/exec.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
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. Review threads the coder judged:
Notes from the agent: Revise round 3. (1) relay_box_channel now holds the stdin task in an AbortOnDrop guard, so every return path aborts it, the |
There was a problem hiding this comment.
Re-reviewed at da9de7e. da9de7e: the stdin pump is held in an AbortOnDrop guard, so every early return aborts it (z0Oei); the e2e comment names #3012 (z0OwJ); the no-terminal refusal fills in the box, keeps one fact per sentence, and its test is renamed box_exec_tty_refuses_without_a_terminal while box_exec_tty_prints_exec_id_to_stderr still matches the plan filter. Earlier: -t refuses before ensure_daemon with exit 2 and always attaches a real terminal (C1), and the not-running refusal names the box's state (C2).
|
The runtime will not revise this pull request again on its own: the diff is +1775/-14 (1789 lines), past 3x the plan's estimate of 580 lines (est: 280 code, 300 tests, as #2905 reads now), at round 1 (the last trigger was a conflict with the base branch). A person decides now. Remove |
|
/review |
…and-in-a-running-box # Conflicts: # crates/minimal/src/cli.rs # crates/minimal/src/cmd/mod.rs
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The merge from main keeps both sides of the two conflicts. It changes no line of the branch. min box exec still conforms to architecture.md line 497 and the exit-code table. One condition stays open. A session e2e lane must run box_exec on this head and report box_exec_tty as passed. That step is the only proof of the real-terminal -t path. C1 states it.
Ruled at 0b39fff against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
A merge of main before round 1, not counted against the cap (1 of 3), pushed as Resolved conflicts in:
Verify Outside the task's
Converted to a draft: something above needs a person before review.
Notes from the agent: Conflict round only. The four conflicts in cli.rs and cmd/mod.rs were additive: this branch added the Exec verb, and main added the Rename and Prune verbs at the same places. The merge commit keeps both sides. cargo clippy -p minimal --all-targets is clean after the merge. The other files in the merge commit come from main and are not changes of this task. |
|
The runtime will not revise this pull request again on its own: the diff is +1775/-14 (1789 lines), past 3x the plan's estimate of 580 lines (est: 280 code, 300 tests, as #2905 reads now), at round 1 (the last trigger was a conflict with the base branch). A person decides now. Remove |
|
/review |
Keep box exec beside the new box events and box cp verbs.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The branch lines did not change since 0b39fff4. The new commit is only a merge of main, and the conflict resolution keeps both sides whole. min box exec conforms to the architecture's min box exec entry and to BCLI-053. One condition stays open. No crate test drives -t on a real terminal, and only proof_box_exec in the session e2e proves it. Link a passing e2e run on this head, with box_exec_tty run and not not_run, to meet C1.
Ruled at 4165a9d against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
A merge of main before round 1, not counted against the cap (1 of 3), pushed as Resolved conflicts in:
Verify Outside the task's
Converted to a draft: something above needs a person before review.
Notes from the agent: This round resolved the merge conflict with main and made no other change. Each conflict kept both sides: the Exec verb and the Events and Cp verbs in cli.rs and cmd/mod.rs, the exec and file refusal exit codes and their tests in main.rs, and the exec test harness and the events test helpers in tests.rs. The merge commit also carries the files that merged cleanly from main (Cargo.lock, bep, minimal-client, cmd/boxes.rs, cmd/boxes/events.rs). These are base changes, not task work. |
Pull request was converted to draft
|
The runtime will not revise this pull request again on its own: the diff is +1775/-14 (1789 lines), past 3x the plan's estimate of 580 lines (est: 280 code, 300 tests, as #2905 reads now), at round 1 (the last trigger was a conflict with the base branch). A person decides now. Remove |
T15 — Run a command in a running box
slice: S4
kind: task
covers: BCLI-053
size: L
est: 280 code, 300 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: when the box's spec turns exec off,
min box execprints " does not allow exec (its spec sets [io] exec_enabled = false)." on stderr and exits 5; for a box that is not running it prints " is not running." and exits 2, pinned by box_exec_runs_command_and_exits_with_its_codeobservability: the command'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/mod.rs: dispatches the verb
- crates/minimal/src/cmd/boxes/exec.rs:
min box exec <box> [-t|--detach] [--timeout <d>] -- <cmd>over the exec operation- 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_exec_runs_command_and_exits_with_its_code
test: cargo nextest run -p minimal box_exec_tty_prints_exec_id_to_stderr
test: cargo nextest run -p minimal box_exec_detach_prints_id_and_returns
test: cargo nextest run -p minimal box_exec_timeout_flag_requests_exec_timeout
test: cargo nextest run -p minimal exec_timeout_returns_124_from_exec_and_wait_exec
produces: none
consumes:
- crates/minimal/src/cmd/boxes/address.rs
proof:
min box execruns a command in a running box, with a terminal or detached, and exits with the command's code.depends on:
- blocked by #2895
- blocked by #2406
Proof
A person can run
min box exec <box> -- <cmd>against a running box. The command's stdout and stderr are relayed and min exits with the command's code. With -t, min prints the exec id on stderr and connects the terminal to the command. With --detach, min prints only the exec id on stdout and returns. --timeout 30s|5m|1h asks for the exec's own timeout, and a timed-out exec ends with exit 124. A box that is not running gives " is not running." and exit 2. A box whose spec sets [io] exec_enabled = false gives " does not allow exec (its spec sets [io] exec_enabled = false)." and exit 5.Notes from the agent: Evidence limit: the minimald test harness cannot spawn a command when another crate drives it. Its real sandbox launcher fails with 'no such package: base', and the mock launcher is cfg(test) inside minimald only. So the harness tests prove the refusals against a real daemon: exit 2 and exit 5, in all three modes, with nothing run. The run, -t, --detach and timeout paths are proved over scripted exec channels. They go through an ExecChannel/ExecChannels trait that the real russh channel implements, so the tests use the same relay code as the real command. No test in this crate runs a real command end to end, and none drives the real-terminal -t path (ssh -tt plus the tty relay). An e2e or test-vm run must cover those.
Design: the exec talks to the daemon over the client's own channel, not over an ssh subprocess, except -t on a real terminal. That path starts the exec in-process, then attaches with
ssh -ttandmin://exec/attach <id>through the existing terminal relay. Off a terminal, -t is refused with exit 2 before min connects, and nothing starts (the architecture'smin box execentry). The VM row resume and the name re-hold thatmin session execdoes still run first.Refusal detection: the daemon reports a refusal as exit 2 or 5 plus its own stderr text. In attached mode a command can exit 2 or 5 too. So the client holds back a first stderr chunk only when it matches the daemon's refusal text (c…[truncated]
Tests
cargo nextest run -p minimal box_exec_runs_command_and_exits_with_its_codecargo nextest run -p minimal box_exec_tty_prints_exec_id_to_stderrcargo nextest run -p minimal box_exec_detach_prints_id_and_returnscargo nextest run -p minimal box_exec_timeout_flag_requests_exec_timeoutcargo nextest run -p minimal exec_timeout_returns_124_from_exec_and_wait_execVerify
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.Residuals
proof_box_execin scripts/session-e2e.sh runsmin box execend to end (plain exec and its exit code,-ton a real pty and off a terminal,--detach,--timeout→ 124, a stopped box).Closes #2905
Closes #3012