Stop background terminal by#1
Open
no-on3 wants to merge 3 commits into
Open
Conversation
… by id
Introduces a new protocol op that terminates a single background terminal
process by its unified-exec id. The existing Op::CleanBackgroundTerminals
continues to stop everything, and its wire shape is unchanged.
- protocol: add `StopBackgroundTerminal { process_id }` variant
- core: add `UnifiedExecProcessManager::terminate_process(i32) -> bool`
mirroring `terminate_all_processes` for a single entry
- core: add `Session::close_unified_exec_process` wrapper and a
`handlers::stop_background_terminal` submission-loop handler that emits
an `EventMsg::Error` when the id is unknown or cannot be parsed
- core tests: add integration coverage that spawns two real processes,
stops one by id, asserts OS-level exit of the target and survival of
the sibling, then verifies the bogus-id error path
Mirrors the existing thread/backgroundTerminals/clean method so the TUI (and other JSON-RPC clients) can target a single background terminal process by id. The method is gated behind experimentalApi, matching the rest of the thread/backgroundTerminals surface. - app-server-protocol: add ThreadBackgroundTerminalStop request/response types and the #[experimental] client_request definition - app-server: add thread_background_terminal_stop handler that loads the thread and submits Op::StopBackgroundTerminal on its behalf - README: document the new method and add an invocation example
Users previously had `/stop` (alias `/clean`) to terminate all background terminals at once. This commit adds `/stop <id-or-prefix>` as an inline argument, keeping bare `/stop` (stop-all) behavior unchanged. - slash_command: opt Stop into supports_inline_args and update its help description to mention the optional id - history_cell: extend UnifiedExecProcessDetails with a `short_id` and render each `/ps` bullet as ` • [short_id] command`, accounting for the id width in the truncation math - chatwidget: derive short_id from the full process key (first 6 graphemes) and add `stop_one_background_terminal` with prefix-based matching (unique match -> submit; 0 matches or >1 -> user-facing error). Local `unified_exec_processes` is pruned optimistically. - slash_dispatch: route `/stop <arg>` through the new helper and drain pending submission state like other inline-arg commands - app_command/app/app_server_session: plumb a new AppCommandView variant and call the app-server's thread/backgroundTerminals/stop method - tests: cover stop-all regression, exact id, unique prefix, ambiguous prefix, unknown id, and composer-clearing after dispatch - snapshots: regenerate /ps insta snapshots to include [short_id] prefix
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
1 participant
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.
What?
Today
/stop(alias/clean) in the TUI terminates all running background terminals at once. This PR adds a second form,/stop <id-or-prefix>, that stops a single terminal./stopwith no arguments keeps its existing stop-all behaviour.Why?
Users that launch multiple long-lived background shells (dev servers, log tails, watchers) currently have no way to stop just one of them — the only option is to nuke everything and restart the ones they still want. This adds targeted control without changing the existing stop-all semantics.
How?
Commits are split by crate and each is individually atomic:
core: add Op::StopBackgroundTerminal— new protocol variant,UnifiedExecProcessManager::terminate_process(i32) -> boolmirroring the existingterminate_all_processes, session wrapper, and a submission-loop handler that emitsEventMsg::Errorwhen the id is unknown or cannot be parsed. ExistingOp::CleanBackgroundTerminalsis untouched and wire-compatible.app-server: expose thread/backgroundTerminals/stop— new experimental JSON-RPC method mirroring the existingthread/backgroundTerminals/clean. The TUI routes ops through the app-server, so this is required for the slash command to work end-to-end.tui: add /stop <id>— optsStopintosupports_inline_args, extendsUnifiedExecProcessDetailswith ashort_idand renders/psbullets as• [short_id] command, and adds astop_one_background_terminalhelper with prefix-based matching: unique prefix → submit + prune local cache + info message; zero or multiple matches → user-facing error.Design notes
i32s ([1000, 100000))./psshows the first 6 graphemes of the string form as a "short id" and/stop <arg>accepts anything that is a prefix of the full key, so users can type as few characters as they need while still pasting a full id from logs. Considered and rejected an ephemeral per-/psalias table (1,2, …) because it would make/stop 1dependent on the most recent/psinvocation.Opvariant.Op::StopBackgroundTerminalis deliberately distinct fromCleanBackgroundTerminalsso the existing stop-all wire format stays byte-compatible for clients that depend on it.add_error_message), consistent with how other slash commands report invalid input.Test plan
cargo test -p codex-core— new integration testunified_exec_stop_background_terminal_by_id_and_report_unknownspawns two real processes viaprintf '%s' $$ > pidfile && exec sleep 3000, stops one by id, waits for OS-level exit of the target, asserts the sibling is still alive, then verifies the bogus-id error path withi32::MAX(guaranteed not to collide with generated ids in[1000, 100000)).cargo test -p codex-tui— new slash-command tests cover: stop-all still works with empty args, exact-id submission, unique-prefix submission, ambiguous-prefix error (op channel empty, local cache intact), unknown-id error, composer clearing after dispatch throughsubmit_composer_text.cargo insta test -p codex-tui -- ps_output— regenerated the fourps_output_*_snapshotinsta snapshots to show the new[short_id]prefix and the adjusted truncation budget.Manual TUI smoke test: start two
sleep 9999background commands,/psto see ids,/stop <short-id>for one, confirm only that one disappears,/stopto drop the remaining one.Scope notes
thread/backgroundTerminals/stopunder the existing experimental capability. No breaking changes./clean <id>(the legacy alias) now also works, becauseStopopts into inline args.ChatWidget;/stop 1by index is explicitly not supported.