fix(exec): report real command exit codes instead of a synthetic -1 - #1882
Merged
bobleer merged 2 commits intoJul 30, 2026
Merged
Conversation
Remote workspace commands finished successfully but were reported to the model as `exit_code: -1`. `run_ssh_channel` treated `SSH_MSG_CHANNEL_EOF` as the end of the channel. EOF only means the peer will send no more data; RFC 4254 6.10 does not fix the ordering of the `exit-status` request, and OpenSSH flushes EOF from its channel loop before it reaps the child and reports the status. Breaking on EOF therefore discarded the exit code of nearly every short-lived command, and `workspace_pipe_owner` then turned the resulting unknown status into a synthetic `-1` that is indistinguishable from a real failure. The one-shot path in `execute_command_internal` already drained until the stream ended, which is why shell probing and env snapshots kept working. Both SSH channel owners now keep draining after EOF until CHANNEL_CLOSE, with a bounded grace so a server that goes quiet cannot wedge them, and they break immediately once the status arrives so the normal path adds no latency. An unknown status is reported as unknown rather than as -1, and `exit-signal` maps to the conventional `128 + signal` status in the channel owners and in the one-shot path, which previously dropped it entirely. Signal deaths of a supervised `docker exec` child map the same way instead of losing `ExitStatus::code() == None`. The local pipe path had an independent defect. `request_control` takes the terminator, so the control sender is dropped as soon as the first interrupt or kill is queued; the unguarded `control_rx.recv()` select branch then resolved to `None` instantly and forever. With `biased` ordering that starved the reader-done branch, so the loop never reached its exit condition: the session stayed open with no exit code while re-signalling a dead process group in a hot loop. The branch is now disarmed once closed, and local signal deaths also report `128 + signal`. Finally, a completed process with no reported code rendered as "Process status unavailable"; it now says so explicitly.
vi.mock factories are hoisted above ordinary module-scope const declarations, so the referenced vi.fn() instances were still in their temporal dead zone when the factory ran, throwing "Cannot access 'saveSessionTurn' before initialization" and failing the whole suite. Use vi.hoisted, matching the pattern already used elsewhere in this codebase (e.g. initializeLsp.test.ts), so the mocks are created before vi.mock needs them.
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.
Summary
Remote workspace commands that ran fine were reported to the model as
exit_code: -1.run_ssh_channeltreatedSSH_MSG_CHANNEL_EOFas the end of the channel:EOF only means the peer will send no more data. RFC 4254 §6.10 does not fix the ordering of the
exit-statusrequest, and OpenSSH'sserver_loop2flushes EOF fromchannel_after_poll()before it reaps the child and sends the status — so for any short-lived command the status arrives after EOF and was thrown away.workspace_pipe_ownerthen turned that unknown status into a synthetic-1, which the model cannot distinguish from a real failure.The one-shot path (
execute_command_internal) already drained until the stream ended, which is why shell probing and env snapshots kept reporting correct codes — the two paths simply disagreed.Auditing the rest of the exec surface turned up several related defects, including an independent one in the local path.
Type and Areas
Type: bug fix
Areas: Rust core — remote SSH workspace exec, local terminal exec, tool result rendering
Motivation / Impact
Every non-TTY remote workspace command reported
exit_code: -1andProcess exited with code -1.to the agent, so a successful command looked like a failure. Agents cannot reason about command success in a remote workspace at all today.Remote
run_ssh_channel/remote_pty_owner: keep draining after EOF untilCHANNEL_CLOSE, with a bounded 5s grace so a server that goes quiet cannot wedge the owner. Both break immediately once the status arrives, so the normal path adds no latency.null) instead of a fabricated-1.exit-signalmaps to the conventional128 + signalstatus (was_ => -1).execute_command_internalpreviously discardedexit-signalentirely and fell through to-1; it now records it too.docker execchild killed by a signal hasExitStatus::code() == None; that is mapped the same way instead of being lost.Local (independent defect found while auditing)
request_controltakes the terminator, so the control sender is dropped as soon as the first interrupt or kill is queued. Thecontrol_rx.recv()select branch inspawn_pipe_processhad no guard, so it then resolved toNoneinstantly and forever. Withbiasedordering that starved the reader-done branch, and the loop never reached its exit condition:The branch is now disarmed once the channel closes, and local signal deaths also report
128 + signal. This fixes fiveterminal-coretests that were failing onmain.Rendering
A completed process with no reported code rendered as
Process status unavailable.; it now saysProcess exited, but no exit code was reported by the transport.No exit code is ever invented.Verification
Fully tested. AI-assisted (Claude Code).
New regression coverage — an in-process
russhserver drives the channel owner through the orderings real servers use, so this no longer depends on having a live host:exit_status_sent_after_eof_is_still_reported— reproduces the bug:None(→-1) before the fix,Some(7)afterexit_status_sent_before_eof_is_reported,exit_signal_sent_after_eof_maps_to_a_conventional_status,missing_exit_status_stays_unknownpipe_owner_reports_{successful,failing}_process_exit_code,..._after_large_output,..._signal_death_as_conventional_statuslocal_process_signal_death_reports_a_conventional_status,ssh_exit_signals_map_to_conventional_wait_statusespipe_exec_reports_signal_death_as_a_conventional_status(local)command_response_says_an_exited_process_had_no_reported_exit_codeTwo exclusions above are pre-existing failures unrelated to this change, listed under Reviewer Notes.
Reviewer Notes
Why the grace window. Waiting for
CHANNEL_CLOSEcosts nothing in practice because it follows the status immediately; the 5s grace only bounds a server that sends EOF and then goes silent without closing. Previously that case broke instantly, so the only behaviour change there is a bounded delay before reporting the same unknown status.Local PTY path left as-is.
portable_pty::ExitStatushard-maps signal death tocode = 1and does not expose the signal name, sotty=truelocal commands cannot distinguish "exited 1" from "killed by signal". It reports a plausible non-zero rather than-1, so I left it rather than guessing; worth a separate look if it matters.Also fixed: unrelated
Frontend BuildCI failure.main's own tip commit (93ba4ece7, the base of this PR) already failed CI atRun web UI tests:PersistenceModule.test.tsthrewCannot access 'saveSessionTurn' before initialization.vi.mockfactories are hoisted above ordinary module-scopeconstdeclarations, so thevi.fn()instances referenced inside the factory were still in their temporal dead zone. Switched tovi.hoisted, the pattern already used elsewhere in this codebase (e.g.initializeLsp.test.ts). Confirmed the fullpnpm --dir src/web-ui exec vitest runsuite is green: 359/359 files, 2377/2377 tests. Unrelated to the exec/exit-code fix above; included here only because it was blocking this PR's CI.Two flaky pre-existing fixtures.
control_interrupt_kills_running_pipe_process_group_after_graceandcontrol_kill_closes_pipe_session_after_parent_exit_with_descendant_pipesfail roughly 2 in 12 full-suite runs on a loaded machine, always on the setup assertion (first.session_id) before any control action is sent — their 500ms/700ms windows are too tight under parallel load. 0/15 failures in isolation and 0/6 with--test-threads=1. Not introduced here (they failed 100% of the time before this fix, at the later assertion), but they could use longer windows.Pre-existing issues found while verifying, not touched:
src/apps/relay-server/tests/library_compat.rs:26—AppStateinitializer is missing thepage_browser_authfield, so that test target does not compile.bitfun-desktoprequiressrc/mobile-web/distto exist, socargo check --workspacefails before the web build.src/crates/services/services-core/src/json_store.rs:346— deny-levelclippy::suspicious_open_options(create(true)withouttruncate), which blocks clippy for every crate depending on it.Checklist