From 13c7a9b8a57b0e34874fb58ac086d362facc770c Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sun, 28 Jun 2026 06:03:40 -0400 Subject: [PATCH] feat(ui): route slash commands and the manual shell in the full-screen app A typed /... or !... line is a harness control, not a model turn. The dock routes it to a SlashRouter instead of the model: fast display commands run on the loop thread against a pane-capturing console (output rendered into the pane); interactive/slow/own-stdout commands (confirm, cloud consent, /doctor, /model use) and the manual shell suspend the app via run_in_terminal; and a model-invoking /plan revise runs on the worker thread like a turn. needs_terminal and needs_worker classify deterministically, with a _decline loop-path confirm safety net so a misclassified command can never block the event loop. Adds DESIGN section 31.17. --- docs/DESIGN.md | 16 ++ shellpilot/cli/app.py | 9 + shellpilot/cli/app_main.py | 4 +- shellpilot/cli/app_slash.py | 154 +++++++++++++++ shellpilot/cli/app_turn.py | 39 ++++ shellpilot/cli/app_ui.py | 11 ++ shellpilot/cli/slash.py | 79 ++++++++ shellpilot/cli/terminal.py | 94 ++++++++-- tests/test_app.py | 54 ++++++ tests/test_app_slash.py | 360 ++++++++++++++++++++++++++++++++++++ tests/test_app_turn.py | 50 +++++ 11 files changed, 855 insertions(+), 15 deletions(-) create mode 100644 shellpilot/cli/app_slash.py create mode 100644 tests/test_app_slash.py diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 1916b21..dff9e2b 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -2879,6 +2879,22 @@ Approvals are the exception to the fire-and-forget marshaling of §31.13: `ask_a **Shutdown ceiling.** A pending approval at app exit leaves the daemon worker blocked on `future.result()`; harmless for this opt-in dev entry (process exit reaps it). On promotion to the shipping default, resolve pending approvals as DECLINE on exit. +### 31.17 Slash-command routing (v2) + +A typed `/...` or `!...` line is a harness control, not a model turn. The dock's submit keybinding (after the §31.16 approval-gate check and the `/exit` quit) routes any such line to the `SlashRouter` (`cli/app_slash.py`) via the `on_slash` callback, instead of sending it to the model. `/exit` stays a direct quit; a normal line still starts a turn. The router splits the work by what would **hang** the loop. + +**The invariant is "never hang", not "never run synchronous code".** The event loop must not block on anything that can stall for a *user-perceptible* or *unbounded* time — a model call or a network call (a hung Ollama would otherwise freeze the TUI for the client timeout, with Ctrl-C dead because it is a keypress the blocked loop never reads). It runs fast, bounded, local work inline like any keybinding: rendering, and the small-file reads behind display commands (`/logs`, `/memory show`, `/config reload`, `/export`) — the same synchronous handling the default REPL ships, completing in sub-millisecond-to-low-millisecond time with no hang. So the dividing line for off-loop execution is *network/model*, deliberately not *all I/O*. + +**Fast display commands run on the loop thread.** A non-interactive slash command that does only in-memory work or a small local-file read (`/help`, `/status`, `/context`, `/logs`, `/memory show`, `/config reload`, …) runs against a fresh pane-capturing `rich.Console` (force-terminal, truecolor, at the current pane width); the captured ANSI is parsed back with `Text.from_ansi` and appended to the pane by `AppUI.show_slash_output`. No suspension, no terminal hand-off. + +**Interactive / slow / own-stdout commands run via `run_in_terminal`.** A command that calls `confirm()`, prompts for cloud consent, prints to its own `Console()` (`/doctor` → `run_doctor`), or does slow preload work (`/model use`) must reach the real terminal. `slash.needs_terminal(line)` classifies these deterministically (`/shell`, `/clear`, `/doctor`, `/plan cancel`, `/cwd set`, `/config set|reset`, `/memory add|forget|compact`, `/model use`); the router schedules them under `run_in_terminal`, which suspends the full-screen app, restores the real terminal, runs the handler synchronously, then redraws. `needs_terminal` enumerates every confirm/consent/own-stdout/preload form — a new one must be added there too. + +**Blocking commands run on the worker thread.** Two kinds of slash command block and so cannot run on the loop thread. (1) `/plan revise ` calls `runtime.run_turn` — a model turn; running it on the loop thread would freeze the UI and the §31.16 approval-gate `Future` could only be resolved by the now-blocked loop → deadlock, and `run_in_terminal` would suspend the app while the turn marshals UI to it. `slash.needs_worker(line)` selects it (only `/plan revise` with non-empty text; a bare `/plan revise` just prints usage on the loop path); the router echoes the line (it is a turn) and runs it via `TurnRunner.start_action`, so its output reaches the pane through the marshaling UI and its approvals use the focus-swap gate. (2) `/model list` (`GET /api/tags`) and `/attach ` (`POST /api/show`) make a blocking Ollama call but need no real terminal; `slash.needs_background(line)` selects them. Both kinds run on the same worker (`start_action`); the background commands are NOT echoed and their captured output is marshaled into the pane (`schedule` → `show_slash_output`). The criterion is "blocks the loop", not just confirm/consent — a hung Ollama must never freeze the TUI for the client timeout. A new command that drives `run_turn` goes in `needs_worker`; one that makes a blocking network/IO call goes in `needs_background`. + +**One dispatcher, two consoles, a confirm safety net.** Both the loop and terminal paths drive the app's single `SlashDispatcher`. The injected `dispatch(line, console)` closure swaps the dispatcher's console per call: the loop path passes the capturing console; the terminal path passes the real console. It also swaps the confirm — the loop path uses `_decline` (always declines, never `input()`), so a slash form misclassified as fast can never block the event loop; the terminal path uses the real blocking `_default_confirm`. Both are restored in `finally`. + +**`!` / `/shell` and the busy guard.** `!` / bare `!` / `/shell` always run on the real terminal (`run_in_terminal` → `run_manual_command` / `manual_shell_loop`, at the live workspace so a prior `/cwd set` is honoured). A slash submitted while a turn is in flight is rejected with a status line (`is_busy` reads `TurnRunner.busy` on the loop thread) — single model, single turn. The default (non-app) REPL is untouched: it routes slash/`!`/manual-shell exactly as before. + ## 32. Model Selection And Preload ### 32.1 Boot Model Picker diff --git a/shellpilot/cli/app.py b/shellpilot/cli/app.py index 50f1c3c..9fffedb 100644 --- a/shellpilot/cli/app.py +++ b/shellpilot/cli/app.py @@ -155,6 +155,7 @@ def build_app( ui: AppUI | None = None, on_submit: Callable[[str], None] | None = None, on_interrupt: Callable[[], bool] | None = None, + on_slash: Callable[[str], None] | None = None, approval_gate: ApprovalGate | None = None, ) -> Application[None]: """Build the full-screen app shell. @@ -313,6 +314,14 @@ def _submit(event: KeyPressEvent) -> None: if text.strip() == "/exit": event.app.exit() return + stripped = text.strip() + if on_slash is not None and stripped and stripped[0] in "/!": + # A typed slash or `!` line is a harness control, not a model turn: + # route it to the SlashRouter (capture / run_in_terminal / manual + # shell / exit). /exit stays a direct quit (handled above), §31.17. + dock_buffer.reset() + on_slash(text) + return if text.strip(): if on_submit is not None: # Echo the typed line into the pane on the loop thread, then run diff --git a/shellpilot/cli/app_main.py b/shellpilot/cli/app_main.py index edff8cc..07a9221 100644 --- a/shellpilot/cli/app_main.py +++ b/shellpilot/cli/app_main.py @@ -13,7 +13,7 @@ from __future__ import annotations -from collections.abc import Sequence +from collections.abc import Callable, Sequence from pathlib import Path from typing import TYPE_CHECKING @@ -40,6 +40,7 @@ def run_app( is_cloud: bool = False, ctx_pct: int = 0, approval_gate: ApprovalGate | None = None, + on_slash: Callable[[str], None] | None = None, ) -> int: """Build the full-screen app around an already-wired conversation and run it. @@ -73,6 +74,7 @@ def run_app( ui=app_ui, on_submit=runner.start, on_interrupt=runner.request_cancel, + on_slash=on_slash, approval_gate=approval_gate, ) runner.app = app diff --git a/shellpilot/cli/app_slash.py b/shellpilot/cli/app_slash.py new file mode 100644 index 0000000..96fa5c7 --- /dev/null +++ b/shellpilot/cli/app_slash.py @@ -0,0 +1,154 @@ +"""Slash / manual-shell routing for the full-screen app (design section 31.17). + +The dock-submit keybinding sends every ``/`` and ``!`` line here on the LOOP +thread (the prompt_toolkit event loop). The event loop owns the terminal and +must never block, so this router splits the work two ways: + +* **Fast, display-only** slash commands run on the loop thread against a fresh + pane-capturing :class:`~rich.console.Console`; the captured ANSI is pushed + into the pane (``AppUI.show_slash_output``). +* **Interactive / slow / own-stdout / manual-shell** commands run via + ``run_in_terminal`` (the app suspends, the real terminal is restored, the + handler runs synchronously, then the app redraws) so ``confirm()`` / + cloud-consent ``input()`` and ``run_doctor``'s own stdout work, and a slow + model preload never freezes the TUI. :func:`~shellpilot.cli.slash.needs_terminal` + classifies which form needs the real terminal. + +The router is built to be testable by INJECTING its effects (``dispatch``, +``run_terminal``, ``manual_shell``, ``on_exit``, ``is_busy``) — no running +prompt_toolkit app is needed in CI. +""" + +from __future__ import annotations + +import io +from collections.abc import Callable + +from rich.console import Console + +from shellpilot.cli.app_ui import AppUI +from shellpilot.cli.slash import SlashAction, needs_background, needs_terminal, needs_worker +from shellpilot.cli.theme import SHELLPILOT_THEME, UNICODE_GLYPHS, Glyphs + + +class SlashRouter: + """Routes a dock-submitted ``/`` or ``!`` line to the right execution context. + + ``dispatch(line, console)`` runs the app's single ``SlashDispatcher`` against + the GIVEN console (and the right confirm — see the wiring's loop-path + ``_decline`` safety net): the loop path passes a capturing console, the + terminal path passes ``real_console``. ``run_terminal`` schedules a zero-arg + fn under ``run_in_terminal``; ``manual_shell`` runs a ``!``/``/shell`` line on + the real terminal; ``on_exit`` exits the app; ``is_busy`` reports whether a + turn is in flight (a slash is rejected while busy). + """ + + def __init__( + self, + *, + ui: AppUI, + dispatch: Callable[[str, Console], SlashAction], + real_console: Console, + width_fn: Callable[[], int], + run_terminal: Callable[[Callable[[], None]], None], + run_worker: Callable[[Callable[[], None]], bool], + schedule: Callable[[Callable[[], None]], None], + manual_shell: Callable[[str], None], + on_exit: Callable[[], None], + is_busy: Callable[[], bool], + glyphs: Glyphs = UNICODE_GLYPHS, + ) -> None: + self._ui = ui + self._dispatch = dispatch + self._real_console = real_console + self._width_fn = width_fn + self._run_terminal = run_terminal + self._run_worker = run_worker + # Marshal a callback from the worker thread back onto the loop thread (for + # the worker path's captured output → pane); = TurnRunner.schedule. + self._schedule = schedule + self._manual_shell = manual_shell + self._on_exit = on_exit + self._is_busy = is_busy + self._glyphs = glyphs + + def route(self, line: str) -> None: + # Called on the LOOP thread from the dock submit keybinding. + stripped = line.strip() + if self._is_busy(): + self._ui.show_status("Busy — finish or cancel the current turn first.") + return + if stripped.startswith("!"): + # `!` / bare `!` → manual shell, always via the real terminal. + self._run_terminal(lambda: self._manual_shell(stripped)) + return + if needs_worker(stripped) or needs_background(stripped): + # Off the loop thread, both via TurnRunner.start_action: + # * needs_worker (/plan revise) — a model turn: its output reaches the + # pane via the runtime UI and approvals use the focus-swap gate, so + # echo the command first (mirrors a turn submission). + # * needs_background (/model list, /attach ) — a non-interactive + # blocking network/IO call: NO echo; its captured output IS the + # result and is marshaled to the pane. + # The loop thread must never block on either. Read the width here (loop + # thread) and pass it in — get_app() is unavailable on the worker. + if needs_worker(stripped): + self._ui.show_user_message(stripped) + width = self._width_fn() + self._run_worker(lambda: self._dispatch_worker(stripped, width)) + return + if needs_terminal(stripped): + self._run_terminal(lambda: self._dispatch_terminal(stripped)) + return + self._dispatch_loop(stripped) # fast display command — capture into the pane + + def _capturing_console(self, width: int) -> tuple[Console, io.StringIO]: + buf = io.StringIO() + console = Console( + file=buf, + force_terminal=True, + color_system="truecolor", + theme=SHELLPILOT_THEME, + width=width, + ) + return console, buf + + def _dispatch_loop(self, line: str) -> None: + # Fast, non-interactive: run on the loop thread with a fresh capturing + # console at the current pane width; push captured output to the pane. + console, buf = self._capturing_console(self._width_fn()) + action = self._dispatch(line, console) + self._ui.show_slash_output(buf.getvalue()) + self._after_action(action) + + def _dispatch_worker(self, line: str, width: int) -> None: + # Runs on the WORKER thread (a /plan revise turn or a slow /model list // + # /attach ). Capture the dispatcher's console and marshal any output + # to the pane on the loop thread. For /plan revise the captured output is a + # blank line (the turn's real output streams via the runtime's marshaling + # UI during the call); for the background commands it IS the result. + console, buf = self._capturing_console(width) + action = self._dispatch(line, console) + output = buf.getvalue() + self._schedule(lambda: self._deliver_worker(output, action)) + + def _deliver_worker(self, output: str, action: SlashAction) -> None: + # Loop thread (marshaled from _dispatch_worker): push the captured output + # (blank is ignored by show_slash_output), then handle the action. + self._ui.show_slash_output(output) + self._after_action(action) + + def _dispatch_terminal(self, line: str) -> None: + # Runs inside run_in_terminal (app suspended, real terminal available). + # The dispatch is given the REAL console, so confirm()/consent input() and + # run_doctor's own stdout work. + action = self._dispatch(line, self._real_console) + self._after_action(action) + + def _after_action(self, action: SlashAction) -> None: + if action is SlashAction.EXIT: + self._on_exit() + elif action is SlashAction.MANUAL_SHELL: + # /shell → drop into the manual shell. Already inside run_in_terminal + # (needs_terminal('/shell') is True), so call it directly. + self._manual_shell("/shell") diff --git a/shellpilot/cli/app_turn.py b/shellpilot/cli/app_turn.py index fc6634f..261e629 100644 --- a/shellpilot/cli/app_turn.py +++ b/shellpilot/cli/app_turn.py @@ -181,6 +181,12 @@ def __init__(self, *, inner_ui: AppUI, schedule: Schedule | None = None) -> None # injects a synchronous one. self._schedule: Schedule = schedule if schedule is not None else self.schedule + @property + def busy(self) -> bool: + """Whether a turn is in flight. Read on the loop thread (slash routing + rejects a slash while busy, §31.17), consistent with the _busy invariant.""" + return self._busy + def schedule(self, fn: Scheduled) -> None: """Marshal ``fn`` onto the loop thread, then request one repaint. @@ -233,6 +239,29 @@ def start(self, text: str) -> None: self._thread = threading.Thread(target=self._run, args=(text, cancel), daemon=True) self._thread.start() + def start_action(self, fn: Callable[[], None]) -> bool: + """Run a model-invoking slash command (e.g. ``/plan revise``) on the worker. + + Like :meth:`start` but runs an arbitrary ``fn`` — which itself drives a + model turn through the runtime's marshaling UI and the approval gate — + instead of ``run_turn`` directly. A ``/plan revise`` must NOT run on the + loop thread (it would freeze the UI and the approval-gate Future could + only be resolved by the now-blocked loop) nor under ``run_in_terminal`` + (which suspends the app while the turn marshals to it). Returns False when + a turn is already in flight so the caller can surface a hint. + + # NOTE: no cancel event — a /plan revise turn is not Ctrl-C-cancellable + # yet (it does not thread a cancel into run_turn). Fold it into the cancel + # spine if that gap bites. + """ + if self._busy: + return False + self._busy = True + self._cancel = None + self._thread = threading.Thread(target=self._run_action, args=(fn,), daemon=True) + self._thread.start() + return True + def request_cancel(self) -> bool: """Signal the in-flight turn to abort. Runs on the loop thread (Ctrl-C). @@ -288,6 +317,16 @@ def _run(self, text: str, cancel: threading.Event) -> None: finally: self._schedule(self._mark_done) + def _run_action(self, fn: Callable[[], None]) -> None: + """Worker body for :meth:`start_action`: run ``fn``, surface any failure to + the pane, and always clear busy on the loop thread (mirrors :meth:`_run`).""" + try: + fn() + except Exception as exc: # noqa: BLE001 - surface ANY worker failure to the pane + self._schedule(functools.partial(self._inner_ui.show_error, f"Command failed: {exc}")) + finally: + self._schedule(self._mark_done) + def _mark_done(self) -> None: """Clear the busy flag. Scheduled onto the loop thread, so the flag is only ever mutated there (paired with the set in :meth:`start`).""" diff --git a/shellpilot/cli/app_ui.py b/shellpilot/cli/app_ui.py index 3bcbb1f..6b0f659 100644 --- a/shellpilot/cli/app_ui.py +++ b/shellpilot/cli/app_ui.py @@ -295,6 +295,17 @@ def show_user_message(self, text: str) -> None: echo = f"{self._glyphs.chevron} {_sanitize_line(text)}" self._add_renderable(Text(echo, style="sp.accent")) + def show_slash_output(self, text: str) -> None: + # Slash output rendered by the dispatcher's capturing console (ANSI) + # becomes a pane renderable (§31.17). Text.from_ansi parses the ANSI + # styling back into a Rich Text so the pane re-emits it. _add_renderable + # closes any open response first, so the slash block lands after it. + # NOTE: captured at the call-time width; a later resize will not re-wrap + # this block (acceptable for slash output). + stripped = text.rstrip("\n") + if stripped: + self._add_renderable(Text.from_ansi(stripped)) + def show_status(self, text: str) -> None: self._add_renderable(Text(_sanitize_line(text), style="sp.dim")) diff --git a/shellpilot/cli/slash.py b/shellpilot/cli/slash.py index 906afd7..88c327a 100644 --- a/shellpilot/cli/slash.py +++ b/shellpilot/cli/slash.py @@ -95,6 +95,85 @@ def command_words() -> list[str]: return words +def needs_terminal(line: str) -> bool: + """True when a slash line must run with the real terminal (run_in_terminal): + it confirms, prompts for cloud consent, prints to its own stdout, or preloads. + + The full-screen app (§31.17) runs fast, display-only commands on the loop + thread with a pane-capturing console; the forms below instead call + ``self._confirm`` / the cloud-consent prompt, print to their own + ``Console()`` (``/doctor`` → ``run_doctor``), or do slow preload work + (``/model use``), none of which can run on the event-loop thread. + + NOTE: this enumerates every confirm()/consent/own-stdout/preload command. If + a new one is added (a new ``self._confirm`` call site, a cloud-consent path, + a handler that builds its own console, or a slow preload), add it here too. + """ + parts = line.strip().split() + if not parts: + return False + cmd = parts[0].lower() + sub = parts[1].lower() if len(parts) > 1 else "" + if cmd == "/shell": # manual shell + return True + if cmd == "/clear": # confirm + return True + if cmd == "/doctor": # run_doctor prints to its own Console()/stdout + return True + if cmd == "/plan" and sub == "cancel": # confirm + return True + if cmd == "/cwd" and sub == "set": # confirm + return True + if cmd == "/config" and sub in ("set", "reset"): # confirm + return True + if cmd == "/memory" and sub in ("add", "forget", "compact"): # confirm + return True + if cmd == "/model" and sub == "use": # cloud-consent prompt + slow preload + return True + return False + + +def needs_worker(line: str) -> bool: + """True for a slash command that runs a model turn, so it must execute on the + worker thread — NOT the loop thread (would freeze the UI and the approval-gate + Future could only be resolved by the now-blocked loop) and NOT under + ``run_in_terminal`` (which suspends the app while the turn marshals to it). + + Currently only ``/plan revise `` (it calls ``runtime.run_turn``). A bare + ``/plan revise`` with no text only prints usage, so it stays on the loop path. + + NOTE: if another slash command starts driving ``run_turn``, add it here. + """ + parts = line.strip().split() + return len(parts) >= 3 and parts[0].lower() == "/plan" and parts[1].lower() == "revise" + + +def needs_background(line: str) -> bool: + """True for a NON-interactive slash command that makes a blocking network/IO + call. It must run off the loop thread (the event loop must never block — a + hung Ollama would otherwise freeze the TUI for the client timeout with no + Ctrl-C), but it needs no real terminal (no confirm/consent/own-stdout), so the + router runs it on the worker and marshals the captured output into the pane. + + ``/model list`` (``GET /api/tags``) and ``/attach `` (``POST /api/show`` + for the vision-capability check + image load). A bare ``/attach`` only lists + already-staged images in memory, so it stays on the loop path. + + NOTE: add any other non-interactive command that makes a blocking network/IO + call here — the criterion is "blocks the loop", not just confirm/consent. + """ + parts = line.strip().split() + if not parts: + return False + cmd = parts[0].lower() + sub = parts[1].lower() if len(parts) > 1 else "" + if cmd == "/model" and sub == "list": + return True + if cmd == "/attach" and len(parts) > 1: # /attach probes /api/show + return True + return False + + def render_config(loaded: LoadedConfig, console: Console) -> None: table = Table(title="Resolved configuration") table.add_column("Key") diff --git a/shellpilot/cli/terminal.py b/shellpilot/cli/terminal.py index 080b046..9a15a43 100644 --- a/shellpilot/cli/terminal.py +++ b/shellpilot/cli/terminal.py @@ -6,10 +6,12 @@ import sys import time import uuid +from collections.abc import Callable from pathlib import Path from urllib.parse import urlsplit from prompt_toolkit.application import get_app +from prompt_toolkit.application.run_in_terminal import run_in_terminal from rich.console import Console from rich.markup import escape from rich.padding import Padding @@ -17,6 +19,7 @@ from shellpilot.cli.app_approval import ApprovalGate from shellpilot.cli.app_main import run_app +from shellpilot.cli.app_slash import SlashRouter from shellpilot.cli.app_turn import ThreadedUI, TurnRunner from shellpilot.cli.app_ui import AppUI from shellpilot.cli.attachments import AttachmentError, AttachmentQueue, load_image @@ -43,7 +46,12 @@ from shellpilot.cli.render import ( tool_result as render_tool_result, ) -from shellpilot.cli.slash import SlashAction, SlashDispatcher, command_words +from shellpilot.cli.slash import ( + SlashAction, + SlashDispatcher, + _default_confirm, + command_words, +) from shellpilot.cli.status_bar import ctx_percent from shellpilot.cli.streaming import AviationSpinner, DiffReveal, ResponseStream from shellpilot.cli.theme import UNICODE_GLYPHS, Glyphs, build_console, resolve_glyphs @@ -407,6 +415,13 @@ def _relative_age(mtime: float, *, now: float | None = None) -> str: return f"{int(delta // 86400)}d ago" +def _decline(_prompt: str) -> bool: + """Loop-path confirm safety net (§31.17): a slash form misclassified as + fast (display-only) can never block the event loop on ``input()`` — it + declines instead. The terminal path uses the real ``_default_confirm``.""" + return False + + def run_interactive( workspace: Path, resume: str | None = None, model_override: str | None = None ) -> int: @@ -648,12 +663,75 @@ def _preload(model_name: str) -> None: tid = escape(restored_plan.task_id) console.print(f"[sp.dim]Active plan restored: {tid} ({restored_plan.status}).[/sp.dim]") + # Slash dispatcher + attachment queue: shared by BOTH the full-screen app + # (the SlashRouter dispatches against this one instance, §31.17) and the + # default REPL below. Hoisted above the app-mode hand-off so the router can + # close over it; every dep was resolved earlier in run_interactive. + attachments = AttachmentQueue() + dispatcher = SlashDispatcher( + runtime=runtime, + client=client, + console=console, + loaded=loaded, + user_config_file=user_file, + reload_config=load, + glyphs=glyphs, + preload=_preload, + attachments=attachments, + tty=tty, + ) + # Hand off to the full-screen app loop (opt-in, design section 31.13). Placed # after the conversation + restore so the worker-thread turn drives the same # fully-configured runtime; the default REPL below is reached only when # app_mode is False, so it stays byte-identical. if app_mode: assert app_runner is not None and app_ui is not None and approval_gate is not None + runner = app_runner # non-optional local for the closures/lambdas below + real_console = console + + def dispatch(line: str, target: Console) -> SlashAction: + # Run the ONE dispatcher against the given console. Loop path (a + # capturing console, not real_console) gets the _decline confirm so a + # misclassified confirm-caller can never block the event loop; + # terminal path gets the real blocking confirm. Restore in finally. + on_loop = target is not real_console + dispatcher._console = target + dispatcher._confirm = _decline if on_loop else _default_confirm + try: + return dispatcher.handle(line) + finally: + dispatcher._console = real_console + dispatcher._confirm = _default_confirm + + def app_manual_shell(line: str) -> None: + # `!` → one audited raw command; bare `!` / `/shell` → the loop. + # Live workspace from the runtime so a prior /cwd set is honoured. + ws = runtime.status().workspace + command = line[1:].strip() if line.startswith("!") else "" + if command: + run_manual_command(command, ws, audit) + else: + manual_shell_loop(console, ws, audit) + + def schedule_terminal(fn: Callable[[], None]) -> None: + # Suspend the app, run fn synchronously on the real terminal, redraw. + # Fire-and-forget: the returned Future is intentionally not awaited. + run_in_terminal(fn) + + router = SlashRouter( + ui=app_ui, + dispatch=dispatch, + real_console=console, + width_fn=lambda: get_app().output.get_size().columns, + run_terminal=schedule_terminal, + run_worker=runner.start_action, + schedule=runner.schedule, + manual_shell=app_manual_shell, + on_exit=lambda: get_app().exit(), + is_busy=lambda: runner.busy, + glyphs=glyphs, + ) return run_app( runtime, app_runner, @@ -669,20 +747,8 @@ def _preload(model_name: str) -> None: runtime.status().budget.model_context_tokens, ), approval_gate=approval_gate, + on_slash=router.route, ) - attachments = AttachmentQueue() - dispatcher = SlashDispatcher( - runtime=runtime, - client=client, - console=console, - loaded=loaded, - user_config_file=user_file, - reload_config=load, - glyphs=glyphs, - preload=_preload, - attachments=attachments, - tty=tty, - ) console.print( render_banner( diff --git a/tests/test_app.py b/tests/test_app.py index 640d581..af2fa8a 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -440,3 +440,57 @@ def test_eof_cancels_active_gate(tmp_path: Path) -> None: inp.send_text("/exit\n") # gate now inactive → quits app.run() assert gate.cancelled == 1 + + +# --- Branch-8 slash/manual-shell routing (§31.17) ----------------------------- + + +def test_slash_and_bang_lines_route_to_on_slash(tmp_path: Path) -> None: + # A typed slash or `!` line goes to the router (on_slash), NOT the model turn + # (on_submit); a normal line still goes to on_submit; /exit still quits. + submits: list[str] = [] + slashes: list[str] = [] + with create_pipe_input() as inp: + ui = AppUI(glyphs=UNICODE_GLYPHS, width_fn=lambda: 80) + app = build_app( + workspace=tmp_path, + model="gemma4:e4b", + profile="balanced", + glyphs=UNICODE_GLYPHS, + commands=command_words(), + input=inp, # type: ignore[arg-type] + output=DummyOutput(), + ui=ui, + on_submit=submits.append, + on_slash=slashes.append, + ) + inp.send_text("/help\n") # → on_slash + inp.send_text("!ls\n") # → on_slash + inp.send_text("just talk\n") # → on_submit (model turn) + inp.send_text("/exit\n") # → quits, never reaches on_slash + app.run() + assert slashes == ["/help", "!ls"] + assert submits == ["just talk"] + + +def test_slash_without_on_slash_falls_through_to_on_submit(tmp_path: Path) -> None: + # Back-compat: with no on_slash wired (the inert fallback), a slash line still + # reaches on_submit exactly as before branch 8. + submits: list[str] = [] + with create_pipe_input() as inp: + ui = AppUI(glyphs=UNICODE_GLYPHS, width_fn=lambda: 80) + app = build_app( + workspace=tmp_path, + model="gemma4:e4b", + profile="balanced", + glyphs=UNICODE_GLYPHS, + commands=command_words(), + input=inp, # type: ignore[arg-type] + output=DummyOutput(), + ui=ui, + on_submit=submits.append, + ) + inp.send_text("/help\n") + inp.send_text("/exit\n") + app.run() + assert submits == ["/help"] diff --git a/tests/test_app_slash.py b/tests/test_app_slash.py new file mode 100644 index 0000000..92aa905 --- /dev/null +++ b/tests/test_app_slash.py @@ -0,0 +1,360 @@ +"""Tests for slash/manual-shell routing in the full-screen app (§31.17, branch 8). + +The threading + run_in_terminal glue is validated live by the orchestrator; here +the router's effects are INJECTED (no prompt_toolkit needed), so every routing +decision and the pane-capture path are exercised deterministically. +""" + +from __future__ import annotations + +import io + +from rich.console import Console + +from shellpilot.cli.app_slash import SlashRouter +from shellpilot.cli.app_ui import AppUI +from shellpilot.cli.slash import SlashAction, needs_background, needs_terminal, needs_worker +from shellpilot.cli.theme import SHELLPILOT_THEME + +# --------------------------------------------------------------------------- +# needs_terminal — exhaustive TRUE cases + representative FALSE cases +# --------------------------------------------------------------------------- + +# Every slash form that confirms / prompts for consent / prints to its own +# stdout / preloads. Mirrors the self._confirm + cloud-consent + run_doctor +# call sites in slash.py (see the module NOTE in app_slash classification). +_TERMINAL_LINES = [ + "/shell", + "/clear", + "/doctor", + "/plan cancel", + "/cwd set /tmp", + "/config set model.default gemma4:e4b", + "/config reset", + "/memory add be concise", + "/memory forget m-1", + "/memory compact", + "/model use gemma4:e4b", +] + +_LOOP_LINES = [ + "/help", + "/status", + "/plan", + "/plan path", + "/cwd", + "/config show", + "/config unset model.default", + "/config reload", + "/config edit", + "/model", + "/model list", + "/memory show", + "/compact", + "/compact status", + "", + " ", + "hello", +] + + +def test_needs_terminal_true_cases() -> None: + for line in _TERMINAL_LINES: + assert needs_terminal(line) is True, line + + +def test_needs_terminal_false_cases() -> None: + for line in _LOOP_LINES: + assert needs_terminal(line) is False, line + + +def test_needs_terminal_is_case_insensitive() -> None: + assert needs_terminal("/CLEAR") is True + assert needs_terminal("/MODEL USE gemma4:e4b") is True + assert needs_terminal("/Config Set k v") is True + assert needs_terminal("/HELP") is False + + +def test_needs_terminal_tolerates_extra_whitespace() -> None: + assert needs_terminal(" /clear ") is True + assert needs_terminal(" /plan cancel ") is True + assert needs_terminal(" /help ") is False + + +# --------------------------------------------------------------------------- +# SlashRouter — injected effects, no prompt_toolkit +# --------------------------------------------------------------------------- + + +class FakeDispatch: + """Records (line, console) calls; prints a marker to the given console.""" + + def __init__( + self, action: SlashAction = SlashAction.CONTINUE, output: str = "DISPATCHED" + ) -> None: + self.action = action + self.output = output + self.calls: list[tuple[str, Console]] = [] + + def __call__(self, line: str, console: Console) -> SlashAction: + self.calls.append((line, console)) + console.print(self.output) + return self.action + + +class FakeTerminal: + """Records run_in_terminal fns; runs them inline when ``run`` is True.""" + + def __init__(self, run: bool = True) -> None: + self.run = run + self.fns: list[object] = [] + + def __call__(self, fn: object) -> None: + self.fns.append(fn) + if self.run: + fn() # type: ignore[operator] + + +class FakeWorker: + """Records start_action fns; runs them inline when ``run`` is True. Returns + True (started) like the real ``TurnRunner.start_action``.""" + + def __init__(self, run: bool = True) -> None: + self.run = run + self.fns: list[object] = [] + + def __call__(self, fn: object) -> bool: + self.fns.append(fn) + if self.run: + fn() # type: ignore[operator] + return True + + +def make_router( + *, + dispatch: FakeDispatch | None = None, + terminal: FakeTerminal | None = None, + worker: FakeWorker | None = None, + is_busy: bool = False, +) -> tuple[SlashRouter, AppUI, FakeDispatch, FakeTerminal, list[str], list[int], io.StringIO]: + ui = AppUI(width_fn=lambda: 80) + dispatch = dispatch or FakeDispatch() + terminal = terminal or FakeTerminal() + worker = worker or FakeWorker() + manual_lines: list[str] = [] + exits: list[int] = [] + real_buf = io.StringIO() + real_console = Console(file=real_buf, force_terminal=True, theme=SHELLPILOT_THEME, width=80) + router = SlashRouter( + ui=ui, + dispatch=dispatch, + real_console=real_console, + width_fn=lambda: 80, + run_terminal=terminal, + run_worker=worker, + schedule=lambda fn: fn(), # marshal runs inline in tests + manual_shell=manual_lines.append, + on_exit=lambda: exits.append(1), + is_busy=lambda: is_busy, + ) + return router, ui, dispatch, terminal, manual_lines, exits, real_buf + + +def test_fast_command_captured_into_pane() -> None: + router, ui, dispatch, terminal, _, _, _ = make_router(dispatch=FakeDispatch(output="HELLO-OUT")) + router.route("/help") + # Dispatched once, against a CAPTURING console (not the real one). + assert len(dispatch.calls) == 1 + line, console = dispatch.calls[0] + assert line == "/help" + # Output captured and pushed to the pane. + assert "HELLO-OUT" in ui._render_ansi() + # The fast path never touches run_in_terminal. + assert terminal.fns == [] + + +def test_interactive_command_routes_to_terminal_real_console() -> None: + dispatch = FakeDispatch(output="TERM-OUT") + router, ui, _, terminal, _, _, real_buf = make_router(dispatch=dispatch) + router.route("/clear") # needs_terminal → run_in_terminal + assert len(terminal.fns) == 1 # scheduled under run_in_terminal + # The fn ran (FakeTerminal.run=True) and dispatched against the REAL console. + assert len(dispatch.calls) == 1 + assert dispatch.calls[0][1] is router._real_console + # Output went to the real terminal, NOT loop-captured into the pane. + assert "TERM-OUT" in real_buf.getvalue() + assert "TERM-OUT" not in ui._render_ansi() + + +def test_bang_command_runs_manual_shell_via_terminal() -> None: + router, _, dispatch, terminal, manual_lines, _, _ = make_router() + router.route("!ls -la") + assert len(terminal.fns) == 1 + assert manual_lines == ["!ls -la"] + assert dispatch.calls == [] # the dispatcher is never consulted for `!` + + +def test_bare_bang_runs_manual_shell_via_terminal() -> None: + router, _, _, terminal, manual_lines, _, _ = make_router() + router.route("!") + assert len(terminal.fns) == 1 + assert manual_lines == ["!"] + + +def test_shell_command_drops_into_manual_shell() -> None: + # /shell is needs_terminal; the dispatch returns MANUAL_SHELL, and the router + # (already inside run_in_terminal) calls manual_shell("/shell"). + dispatch = FakeDispatch(action=SlashAction.MANUAL_SHELL, output="") + router, _, _, terminal, manual_lines, _, _ = make_router(dispatch=dispatch) + router.route("/shell") + assert len(terminal.fns) == 1 + assert manual_lines == ["/shell"] + + +def test_busy_rejects_and_does_not_dispatch() -> None: + router, ui, dispatch, terminal, manual_lines, exits, _ = make_router(is_busy=True) + router.route("/help") + assert dispatch.calls == [] + assert terminal.fns == [] + assert manual_lines == [] + assert exits == [] + assert "Busy" in ui._render_ansi() + + +def test_exit_action_calls_on_exit() -> None: + dispatch = FakeDispatch(action=SlashAction.EXIT, output="") + router, _, _, _, _, exits, _ = make_router(dispatch=dispatch) + router.route("/help") # fast path; dispatch returns EXIT + assert exits == [1] + + +# --------------------------------------------------------------------------- +# needs_worker + the worker path (/plan revise runs a model turn) +# --------------------------------------------------------------------------- + + +def test_needs_worker_true_for_plan_revise_with_text() -> None: + assert needs_worker("/plan revise make it shorter") is True + assert needs_worker("/PLAN REVISE x") is True + assert needs_worker(" /plan revise y ") is True + + +def test_needs_worker_false_otherwise() -> None: + for line in ["/plan revise", "/plan", "/plan cancel", "/plan path", "/help", "", "hello"]: + assert needs_worker(line) is False, line + # /plan revise must NOT also be classified as a terminal command. + assert needs_terminal("/plan revise make it shorter") is False + + +def test_plan_revise_routes_to_worker_not_loop_or_terminal() -> None: + worker = FakeWorker(run=True) + dispatch = FakeDispatch(output="") + router, ui, _, terminal, manual_lines, _, _ = make_router(dispatch=dispatch, worker=worker) + router.route("/plan revise make it shorter") + # Routed to the worker — NOT loop-capture's run_in_terminal, NOT manual shell. + assert len(worker.fns) == 1 + assert terminal.fns == [] + assert manual_lines == [] + # Echoed as a user message (mirrors a turn submission). + assert "/plan revise make it shorter" in ui._render_ansi() + # The worker fn dispatched against a CAPTURING console (not the real one). + assert len(dispatch.calls) == 1 + assert dispatch.calls[0][1] is not router._real_console + + +def test_plan_revise_without_text_stays_on_loop() -> None: + worker = FakeWorker() + router, _, dispatch, terminal, _, _, _ = make_router(worker=worker) + router.route("/plan revise") # no instruction → usage message, loop path + assert worker.fns == [] + assert len(dispatch.calls) == 1 # loop-capture dispatch, not the worker + assert terminal.fns == [] + + +def test_busy_rejects_plan_revise() -> None: + worker = FakeWorker() + router, ui, _, _, _, _, _ = make_router(worker=worker, is_busy=True) + router.route("/plan revise make it shorter") + assert worker.fns == [] + assert "Busy" in ui._render_ansi() + + +# --------------------------------------------------------------------------- +# needs_background + the worker-display path (blocking network/IO, output→pane) +# --------------------------------------------------------------------------- + + +def test_needs_background_true_for_network_commands() -> None: + assert needs_background("/model list") is True + assert needs_background("/MODEL LIST") is True + assert needs_background("/attach /tmp/cat.png") is True + + +def test_needs_background_false_otherwise() -> None: + for line in ["/attach", "/model", "/model use gemma4:e4b", "/help", "/status", "", "hello"]: + assert needs_background(line) is False, line + # These run a blocking call but must NOT also be loop/terminal-classified. + assert needs_terminal("/model list") is False + assert needs_worker("/model list") is False + + +def test_background_command_runs_on_worker_and_captures_to_pane() -> None: + worker = FakeWorker(run=True) + dispatch = FakeDispatch(output="MODEL-TABLE") + router, ui, _, terminal, manual_lines, _, real_buf = make_router( + dispatch=dispatch, worker=worker + ) + router.route("/model list") + # Ran on the worker — NOT the loop-thread fast path, NOT run_in_terminal. + assert len(worker.fns) == 1 + assert terminal.fns == [] + assert manual_lines == [] + # Captured against a capturing console (not the real terminal) and marshaled + # into the pane; nothing went to the real terminal. + assert len(dispatch.calls) == 1 + assert dispatch.calls[0][1] is not router._real_console + assert "MODEL-TABLE" in ui._render_ansi() + assert "MODEL-TABLE" not in real_buf.getvalue() + # A background DISPLAY command is NOT echoed as a user message (unlike a turn). + assert "❯ /model list" not in ui._render_ansi() + + +def test_bare_attach_stays_on_loop() -> None: + worker = FakeWorker() + router, _, dispatch, terminal, _, _, _ = make_router(worker=worker) + router.route("/attach") # no path → in-memory list, loop path + assert worker.fns == [] + assert len(dispatch.calls) == 1 # loop-capture dispatch + assert terminal.fns == [] + + +# --------------------------------------------------------------------------- +# AppUI.show_slash_output +# --------------------------------------------------------------------------- + + +def test_show_slash_output_appends_ansi_renderable() -> None: + ui = AppUI(width_fn=lambda: 80) + before = len(ui._renderables) + ui.show_slash_output("\x1b[31mhello\x1b[0m\n") + assert len(ui._renderables) == before + 1 + assert "hello" in ui._render_ansi() + + +def test_show_slash_output_closes_open_response() -> None: + ui = AppUI(width_fn=lambda: 80) + ui.stream_token("partial reply") + ui.show_slash_output("DONE\n") + # The open response was committed (closed) before the slash output appended. + assert ui._open_response is None + rendered = ui._render_ansi() + assert "partial reply" in rendered + assert "DONE" in rendered + + +def test_show_slash_output_ignores_blank() -> None: + ui = AppUI(width_fn=lambda: 80) + before = len(ui._renderables) + ui.show_slash_output("\n") + ui.show_slash_output("") + assert len(ui._renderables) == before diff --git a/tests/test_app_turn.py b/tests/test_app_turn.py index 13b6c52..7220702 100644 --- a/tests/test_app_turn.py +++ b/tests/test_app_turn.py @@ -240,6 +240,56 @@ def test_full_turn_runs_on_worker_and_marshals(tmp_path: Path) -> None: assert runner._busy is False +def test_start_action_runs_on_worker_and_clears_busy(tmp_path: Path) -> None: + # The /plan revise + /model list slash paths run via start_action. Prove the + # fn runs on a REAL separate thread and busy is cleared only by the MARSHALED + # _mark_done (not inline on the worker) — the real-threading ordering the + # injected-fake router tests can't cover. + app_ui = AppUI(glyphs=UNICODE_GLYPHS, workspace=tmp_path, width_fn=lambda: 80) + q: queue.Queue[Scheduled] = queue.Queue() + runner = TurnRunner(inner_ui=app_ui, schedule=q.put) + + ran = threading.Event() + worker_ident: dict[str, int] = {} + + def action() -> None: + worker_ident["id"] = threading.get_ident() + ran.set() + + assert runner.start_action(action) is True + assert runner._busy is True # set synchronously in start_action + assert runner._thread is not None + runner._thread.join(5.0) + assert not runner._thread.is_alive() + assert ran.is_set() + assert worker_ident["id"] != threading.get_ident() # ran off the test thread + # _mark_done was marshaled, not run inline — busy stays set until the queue drains. + assert runner._busy is True + while not q.empty(): + q.get()() + assert runner._busy is False + + +def test_start_action_rejects_when_busy(tmp_path: Path) -> None: + app_ui = AppUI(glyphs=UNICODE_GLYPHS, workspace=tmp_path, width_fn=lambda: 80) + q: queue.Queue[Scheduled] = queue.Queue() + runner = TurnRunner(inner_ui=app_ui, schedule=q.put) + gate = threading.Event() + + assert runner.start_action(gate.wait) is True + assert runner._busy is True + # A second action is rejected (returns False) while the first is in flight. + second_ran = threading.Event() + assert runner.start_action(second_ran.set) is False + assert not second_ran.is_set() + gate.set() + assert runner._thread is not None + runner._thread.join(5.0) + while not q.empty(): + q.get()() + assert runner._busy is False + + def test_busy_guard_ignores_second_start(tmp_path: Path) -> None: gate = threading.Event() entered = threading.Event()