From 30236f81ab6c87ef857ac12ef748dec88efd15b8 Mon Sep 17 00:00:00 2001 From: wliao Date: Sun, 6 Sep 2026 16:25:00 +0800 Subject: [PATCH 1/2] Open a side conversation beside its parent, and hand findings back through a compose buffer --- DESIGN.md | 69 ++++++- README.md | 56 ++++-- agent-shell-side.el | 260 +++++++++++-------------- tests/agent-shell-side-tests.el | 327 +++++++++++++++++++------------- 4 files changed, 412 insertions(+), 300 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 627b268..8fc91da 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1,6 +1,6 @@ # Design: open work for agent-shell-side -Status as of 2026-09-06. `make check` is green (99 ERT tests against stubs) +Status as of 2026-09-06. `make check` is green (104 ERT tests against stubs) and `make live-check` passes 10 of 10 against a real claude-agent-acp 0.70. Four questions were open when this was written. This document records the @@ -350,6 +350,73 @@ and what runs either side of it. resumed shell reads its id from agent-shell's state. The test was passing against a source the real path never uses. +## 7. Where it is shown, and one handback path + +Two changes from using the thing, 2026-09-06. + +### Beside the parent, not instead of it + +The package had no display option of its own: it reused +`agent-shell-display-action`, whose default is +`display-buffer-same-window`, so a side conversation took over the +parent's window and toggling swapped them. That is Codex's behaviour +because a terminal has one pane. Emacs does not, and the whole appeal of +a side conversation is reading it next to what it forked from. + +`agent-shell-side-display-action` now defaults to a window on the right. +Kept separate from `agent-shell-display-action` deliberately: pointing +that one at a side window would move every shell, not just these. + +Three things had to move with it, all found by pointing a side-window +action at the old code rather than by reading it: + +- **Closing put the parent into the side window.** The close path handed + the side's window to the parent, giving `((PARENT right) (PARENT nil))` + -- the parent displayed twice, once in a strip meant for something + else. A side window is scenery for what it holds, so it is now deleted + with the conversation. +- **Toggling duplicated the side.** With both visible it gave + `((SIDE nil) (SIDE right))`, the parent nowhere. Toggle now selects the + other window when both are on screen. When they are not, it displays + through the action, which reproduces the old swap exactly when that + action is `display-buffer-same-window` -- so the old behaviour is still + one setting away and needs no special case. +- **`delete-other-windows` signals** while a side window is selected + ("Cannot make side window the only window"). That is why the test + helper deletes side windows before calling it. + +The parent is displayed through `agent-shell-display-action`, not the +side action. It is an ordinary shell. + +Under viewport interaction the action is not consulted at all. The +viewport owns its layout, and a compose buffer wedged into a side window +is not an improvement. + +### One handback path + +The handback had two shapes. Under viewport interaction the findings went +straight into the parent's compose buffer, which takes text mid-turn. +Otherwise they went to the shell prompt, which does not, so a busy parent +meant holding the findings, a pending flag, a mode-line marker, a +deferred re-check to work around `error` arriving before the busy flag +clears, and the side conversation closing itself at whatever moment the +parent's turn happened to end. + +All of that is gone. The findings always go to a compose buffer. + +The argument for it is not just that it is less code. That setting +decides where the *user's own prompts* go; it has no business deciding +what happens to findings, and letting it do so gave the two settings +visibly different commands for the same task. The compose buffer is also +strictly better than waiting: it takes the text immediately, mid-turn, +and hands back the three answers that matter from its own keys -- send, +queue behind the running turn, or steer into it. Waiting could only ever +offer the first, and only later. + +Removed with it: `agent-shell-side-handback-submit`, which had become a +setting that could not do anything, since sending is now the compose +buffer's business rather than ours. + ## Order of work 1. Item 1's live test file, including the item 2 probe. It is the diff --git a/README.md b/README.md index 7f7555a..23aa8da 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ one. | Command | Key (in linked buffers) | What it does | | --- | --- | --- | | `agent-shell-side` | | Fork this conversation into a side one and switch to it | -| `agent-shell-side-toggle` | `C-c C-b` | Switch between the side conversation and its parent | +| `agent-shell-side-toggle` | `C-c C-b` | Move between the side conversation and its parent | | `agent-shell-side-conclude` | `C-c C-q` | Summarise the side conversation into the parent, then close it | | `agent-shell-side-dismiss` | `C-c C-k` | Close the side conversation, discarding what it learned | | `agent-shell-side-resume` | | Reopen a side conversation that was kept | @@ -47,19 +47,21 @@ An empty question opens it blank. ### Carrying findings back `agent-shell-side-conclude` (`C-c C-q`) asks the side conversation for a summary -addressed to the parent, waits for it, puts it at the parent's prompt, and closes -the side conversation. The summary is left for you to review rather than sent. -Set `agent-shell-side-handback-submit` to send it instead. - -Where the summary lands follows how you use `agent-shell`. With -`agent-shell-prefer-viewport-interaction` it is appended to the parent's viewport -compose buffer, opened in edit mode so this works while the parent is mid-turn; -the compose buffer's own keys then send, queue, or steer it. Otherwise it goes -to the parent's shell prompt: at once when the parent is idle, or as soon as its -turn ends when it is not. A busy shell cannot take text at its prompt, since -shell-maker appends output at the end of the buffer and would swallow it. The -side conversation stays open until the findings are staged, so a parent that is -killed first loses nothing, and its mode line says the findings are waiting. +addressed to the parent, waits for it, seeds it into a compose buffer for the +parent, and closes the side conversation. + +The summary is never sent for you. It arrives as a draft you can edit, and the +compose buffer's own keys decide what happens to it: `C-c C-c` sends, `C-c C-q` +queues it behind the parent's running turn, `C-c C-i` steers it into that turn, +and `C-c C-k` throws it away. That is the same set of choices you have for any +prompt, which is the point. + +This is one path, not two. It does not depend on +`agent-shell-prefer-viewport-interaction`: that setting decides where your own +prompts go and has no business deciding what happens to findings. A compose +buffer is also the only surface that works while the parent is mid-turn, since a +busy shell cannot take text at its prompt. shell-maker appends output at the end +of the buffer and would swallow it. If the summary comes back empty, or the parent is gone, the side conversation is left open rather than closed on nothing. Customize @@ -71,6 +73,28 @@ finishes, fails, or asks for approval, the side conversation's mode line says so echoes it once. Set `agent-shell-side-report-parent-status` to nil for the mode line only. +### Where it opens + +A side conversation runs beside the conversation it was forked from, so it opens +in a window of its own on the right and the parent stays visible. That is the +arrangement a terminal cannot offer, and the reason `agent-shell-side-display-action` +exists separately from `agent-shell-display-action`, which is left to decide +where ordinary shells go. + +`agent-shell-side-toggle` (`C-c C-b`) then moves point between the two windows +rather than rearranging them. Closing the side conversation deletes its window. + +To get the old behaviour, where the side conversation takes over the parent's +window and toggling swaps the two, set it to same-window: + +```elisp +(setq agent-shell-side-display-action '(display-buffer-same-window)) +``` + +Everything follows from the action: toggling still selects the other window when +both happen to be visible, and falls back to displaying through the action when +they are not. + ### Keeping track of what is open A side conversation lives until you close it, so they collect quietly. @@ -165,8 +189,8 @@ the record stays so you can try again. With `agent-shell-prefer-viewport-interaction` set, or when started from a viewport buffer, the side conversation opens through a viewport as -`agent-shell-fork` would, and the handback goes to the parent's own compose -buffer rather than its shell prompt. +`agent-shell-fork` would, and `agent-shell-side-display-action` is not consulted: +the viewport owns its own layout. The keys above are deliberately *not* bound in a viewport compose buffer. `agent-shell-viewport-edit-mode` already uses `C-c C-k` to discard the draft and diff --git a/agent-shell-side.el b/agent-shell-side.el index 1ac7c32..f07170a 100644 --- a/agent-shell-side.el +++ b/agent-shell-side.el @@ -132,6 +132,32 @@ every close is friction on the common path. (const :tag "Ask each time" ask)) :group 'agent-shell-side) +(defcustom agent-shell-side-display-action + '((display-buffer-in-side-window) + (side . right) + (window-width . 0.4)) + "Display action for a side conversation's own buffer. + +Separate from `agent-shell-display-action', which decides where ordinary +shells go and is left alone: a side conversation is a different thing on +screen. It runs beside the conversation it was forked from, so the +default puts it in a window on the right and keeps the parent visible. +That is the arrangement a terminal cannot offer -- Codex has one pane and +must swap -- and the reason to have this option at all. + +To get the old behaviour, where the side conversation takes over the +parent's window and `agent-shell-side-toggle' swaps the two, set this to +\='(display-buffer-same-window). Everything else follows: toggling +selects the other window when both are visible and falls back to +displaying through this action when they are not. + +Ignored under `agent-shell-prefer-viewport-interaction', and when the +command was issued from a viewport buffer. The viewport owns its own +layout, and a compose buffer wedged into a side window is not an +improvement. See `display-buffer' for the format." + :type '(cons (repeat function) alist) + :group 'agent-shell-side) + (defcustom agent-shell-side-buffer-name-suffix " [side]" "Appended to the agent's buffer and mode-line names in a side conversation." :type 'string @@ -158,16 +184,6 @@ Its answer is what gets handed back to the parent conversation." :type 'string :group 'agent-shell-side) -(defcustom agent-shell-side-handback-submit nil - "Whether handed-back findings are sent in the parent, or left to review. - -When nil, the findings are inserted at the parent's prompt and left -there for you to send, queue, or steer. Only read when the parent is -shown as a plain shell: the viewport compose buffer has no way to submit -on insertion, and leaves sending to its own keys." - :type 'boolean - :group 'agent-shell-side) - (defcustom agent-shell-side-handback-timeout 180 "Seconds to wait for the summary before giving up on a handback. @@ -215,9 +231,6 @@ One of `needs-approval', `finished', `failed', `closed', or nil.") (defvar-local agent-shell-side--session-id-cache nil "Session id captured from the `session-selected' event, when it arrived.") -(defvar-local agent-shell-side--handback-pending nil - "In a side conversation, non-nil while findings wait for the parent's turn.") - (defun agent-shell-side--live-buffer (buffer) "Return BUFFER when it is still live, else nil." (and (bufferp buffer) (buffer-live-p buffer) buffer)) @@ -566,20 +579,14 @@ to use, so this stays safe to call from a mode-line lighter." shell))) (current-buffer))) -(defun agent-shell-side--lighter-string (role status &optional pending) +(defun agent-shell-side--lighter-string (role status) "Return the mode-line lighter for ROLE with parent STATUS. -ROLE is `side' in a side conversation, `parent' in a shell that has one. -PENDING, in a side conversation, means findings are waiting for the -parent's turn to end. It outranks STATUS: a parent that is mid-turn is -why the findings are waiting, so saying both would say the same thing -twice." +ROLE is `side' in a side conversation, `parent' in a shell that has one." (pcase role - ('side (cond - (pending " Side:findings waiting") - ((agent-shell-side--status-label status) - (concat " Side:" (agent-shell-side--status-label status))) - (t " Side"))) + ('side (if-let* ((label (agent-shell-side--status-label status))) + (concat " Side:" label) + " Side")) (_ " Sideā†©"))) (defun agent-shell-side--lighter () @@ -587,8 +594,7 @@ twice." (let ((shell (agent-shell-side--this-shell))) (agent-shell-side--lighter-string (if (agent-shell-side-buffer-p shell) 'side 'parent) - (buffer-local-value 'agent-shell-side--parent-status shell) - (buffer-local-value 'agent-shell-side--handback-pending shell)))) + (buffer-local-value 'agent-shell-side--parent-status shell)))) (defvar-keymap agent-shell-side-mode-map :doc "Keymap for `agent-shell-side-mode'." @@ -796,16 +802,34 @@ before asking the user for anything." (or (map-elt parent-config :mode-line-name) "This agent"))) parent-buffer)) +(defun agent-shell-side--side-window-p (window) + "Return non-nil when WINDOW is one of the frame's side windows. + +Such a window is scenery for what it holds rather than a place to put +things: it cannot be made the only window, and handing it a different +buffer leaves that buffer stuck in a narrow strip." + (and (window-live-p window) + (window-parameter window 'window-side))) + (defun agent-shell-side--display (buffer &optional viewport) - "Show BUFFER the way `agent-shell' shows its own buffers. + "Show side conversation BUFFER, honoring `agent-shell-side-display-action'. With VIEWPORT, or when the user prefers viewport interaction, the buffer -is shown through a viewport as `agent-shell-fork' would show it. -Otherwise it honors `agent-shell-display-action' rather than picking a -window directly, so a side conversation lands where the user already -told `agent-shell' to put shells." +is shown through a viewport as `agent-shell-fork' would show it, and the +action is not consulted." (if (or viewport (agent-shell-side-compat-prefer-viewport-p)) (agent-shell-side-compat-show-in-viewport buffer) + (when-let* ((window (display-buffer buffer agent-shell-side-display-action))) + (select-window window)))) + +(defun agent-shell-side--display-parent (buffer) + "Show parent shell BUFFER where `agent-shell' puts its own buffers. + +Deliberately not `agent-shell-side-display-action': the parent is an +ordinary shell, and the default side action would file it away in the +strip meant for the side conversation." + (if (agent-shell-side-compat-prefer-viewport-p) + (agent-shell-side-compat-show-in-viewport buffer) (when-let* ((window (display-buffer buffer agent-shell-display-action))) (select-window window)))) @@ -857,12 +881,17 @@ cannot be forked costs no typing." (buffer-local-value 'agent-shell-side--side-buffer shell))))) (unless target (user-error "No side conversation linked to this buffer")) - ;; Swap in place when this buffer has a window: toggling is "show the - ;; other end here", not "find somewhere for it". - (if-let* ((window (get-buffer-window (current-buffer)))) - (progn (set-window-buffer window target) - (select-window window)) - (agent-shell-side--display target)))) + ;; Already on screen: toggling is "look at the other end", so move + ;; point rather than rearranging windows. This is the normal case + ;; under the default side-window layout, where both are visible. + (if-let* ((window (get-buffer-window target))) + (select-window window) + ;; Otherwise put it where its kind belongs. With the action set to + ;; `display-buffer-same-window' this reproduces the old swap, since + ;; displaying in the selected window is what a swap was. + (if (agent-shell-side-buffer-p target) + (agent-shell-side--display target) + (agent-shell-side--display-parent target))))) (defun agent-shell-side--resolve-side () "Return the side conversation this buffer is one end of. @@ -895,11 +924,19 @@ mode and strands the findings there unsendable." (when (buffer-live-p side-buffer) (let ((window (get-buffer-window side-buffer))) (kill-buffer side-buffer) - (when (and parent (not parent-shown) (window-live-p window)) - (set-window-buffer window parent)))) + (cond + ((not (window-live-p window)) nil) + ;; A side window belongs to the side conversation, so + ;; it goes when that does. Handing it to the parent + ;; instead would leave the parent showing twice, once + ;; in a strip it was never meant to occupy. + ((agent-shell-side--side-window-p window) + (ignore-errors (delete-window window))) + ((and parent (not parent-shown)) + (set-window-buffer window parent))))) (when (and parent (not parent-shown) (not (get-buffer-window parent))) - (agent-shell-side--display parent))))) + (agent-shell-side--display-parent parent))))) (with-current-buffer side-buffer (when (memq (agent-shell-status) '(busy blocked)) (agent-shell-interrupt t))) @@ -931,108 +968,38 @@ conversation instead, use `agent-shell-side-conclude'." "Return SUMMARY framed for the parent conversation." (format "%s\n\n%s" agent-shell-side-handback-header (string-trim summary))) -(defun agent-shell-side--parent-busy-p (parent-buffer) - "Return non-nil when PARENT-BUFFER is running a turn or waiting on one." - (memq (agent-shell-status :shell-buffer parent-buffer) '(busy blocked))) - -(defun agent-shell-side--stage-in-shell (parent-buffer text) - "Put TEXT at PARENT-BUFFER's prompt, sent only if the user asked for that." - (agent-shell-insert :text text - :submit agent-shell-side-handback-submit - :no-focus t - :shell-buffer parent-buffer)) - -(defun agent-shell-side--stage-when-idle (side-buffer parent-buffer text on-staged) - "Put TEXT at PARENT-BUFFER's prompt once its turn ends, then call ON-STAGED. - -A busy shell cannot take text at its prompt: `shell-maker' appends -output at the end of the buffer, so anything staged there would be -swallowed by the streaming response. The prompt is printed, and busy -cleared, before `turn-complete' is emitted, so the first event that finds -the parent idle is the moment to insert. - -SIDE-BUFFER stays open meanwhile, marked as having findings pending, so -the summary cannot be lost to a parent that is killed first. That case -is reported and leaves the side conversation as it was. - -A turn that fails rather than finishing needs one extra step. `error' -is emitted before the shell clears its busy state, and nothing is -emitted afterwards, so a handler that only tests for an idle parent -would wait for an event that never comes. The clearing happens in the -same call, right after the event is dispatched, so the test is deferred -to the next timer tick rather than run inline." - (let ((token nil) - (settled nil) - (stage nil)) - (with-current-buffer side-buffer - (setq agent-shell-side--handback-pending t) - (force-mode-line-update)) - (setq stage - (lambda () - (unless (or settled (agent-shell-side--parent-busy-p parent-buffer)) - (setq settled t) - (agent-shell-side--unsubscribe parent-buffer token) - (when (buffer-live-p side-buffer) - (with-current-buffer side-buffer - (setq agent-shell-side--handback-pending nil) - (force-mode-line-update))) - (agent-shell-side--stage-in-shell parent-buffer text) - (funcall on-staged)))) - (setq token - (agent-shell-subscribe-to - :shell-buffer parent-buffer - :on-event - (lambda (event) - (unless settled - (pcase (map-elt event :event) - ('clean-up - (setq settled t) - (when (buffer-live-p side-buffer) - (with-current-buffer side-buffer - (setq agent-shell-side--handback-pending nil) - (force-mode-line-update))) - (message - "agent-shell-side: the parent closed before taking the findings; leaving the side conversation open")) - ('error (run-at-time 0 nil stage)) - (_ (funcall stage))))))) - (message "Side conversation: %s is mid-turn; findings will be staged at its prompt when the turn ends" - (buffer-name parent-buffer)) - token)) - -(defun agent-shell-side--deliver-handback (side-buffer parent-buffer text on-delivered) - "Get TEXT to PARENT-BUFFER for the user to send, then call ON-DELIVERED. - -SIDE-BUFFER is the side conversation the findings came from; it is kept -open, and marked, while they wait for a busy parent. - -Where it lands follows how the user works with `agent-shell'. With -`agent-shell-prefer-viewport-interaction', it is appended to the -parent's viewport compose buffer, opened in edit mode so this works -while the parent is mid-turn; the compose buffer's own keys then send, -queue, or steer it. Otherwise it goes to the parent's shell prompt: at -once when the parent is idle, or as soon as its turn ends when it is -not. Nothing here sends on the user's behalf unless -`agent-shell-side-handback-submit' asks for it. - -Never the minibuffer: findings are usually long, and a minibuffer is no -place to review them." - (cond - ((agent-shell-side-compat-prefer-viewport-p) - (agent-shell-side-compat-compose-in-viewport parent-buffer text) - (funcall on-delivered t)) - ((agent-shell-side--parent-busy-p parent-buffer) - (agent-shell-side--stage-when-idle - side-buffer parent-buffer text (lambda () (funcall on-delivered nil)))) - (t - (agent-shell-side--stage-in-shell parent-buffer text) - (funcall on-delivered nil)))) +(defun agent-shell-side--deliver-handback (parent-buffer text) + "Seed TEXT into a compose buffer for PARENT-BUFFER, for the user to send. + +Always a compose buffer, whatever `agent-shell-prefer-viewport-interaction' +says. That setting decides where the user's own prompts go; it has no +business deciding what happens to findings, and letting it do so gave the +two settings visibly different commands. + +A compose buffer is also the only surface that works here. A busy shell +cannot take text at its prompt -- `shell-maker' appends output at the end +of the buffer and would swallow it -- so the alternative was to hold the +findings until the turn ended, which meant waiting on a turn that might +be long, a pending state to track, and the side conversation closing +itself at whatever moment that turn happened to finish. The compose +buffer takes the text immediately and mid-turn, and hands the user the +three answers that matter, from its own keys: send, queue behind the +running turn, or steer into it. None of them is ours to choose. + +Opened with `:edit t' so it is ready to type in even while the parent is +working. Never the minibuffer: findings are usually long, and a +minibuffer is no place to review them." + (agent-shell-side-compat-compose-in-viewport parent-buffer text)) (defun agent-shell-side--finish-handback (side-buffer parent-buffer summary) "Put SUMMARY into PARENT-BUFFER and close SIDE-BUFFER once it is there. An empty summary or a parent that has since gone away leaves the side conversation open: closing it would throw the conversation away and hand -the parent nothing." +the parent nothing. Otherwise the side conversation closes as soon as +the draft exists -- the findings are out of it and in a buffer of their +own by then, and what to do with them is no longer this command's +business." (let ((summary (string-trim (or summary "")))) (cond ((string-empty-p summary) @@ -1043,10 +1010,10 @@ the parent nothing." "agent-shell-side: the parent is gone; leaving the side conversation open")) (t (agent-shell-side--deliver-handback - side-buffer parent-buffer (agent-shell-side--handback-text summary) - (lambda (parent-shown) - (when (buffer-live-p side-buffer) - (agent-shell-side--close side-buffer parent-shown)))))))) + parent-buffer (agent-shell-side--handback-text summary)) + ;; The parent's compose buffer is now in front of the user, so + ;; closing must not display the parent shell over it. + (agent-shell-side--close side-buffer t))))) (defun agent-shell-side--collect-summary (side-buffer parent-buffer) "Gather SIDE-BUFFER's next answer and hand it to PARENT-BUFFER. @@ -1090,11 +1057,13 @@ arrives first would otherwise be lost from the summary." "Summarise the side conversation into the parent, then close it. Asks the side conversation for a summary addressed to the parent, waits -for it, puts it at the parent's prompt, and closes the side conversation -the way `agent-shell-side-dismiss' would. +for it, seeds it into a compose buffer for the parent, and closes the +side conversation the way `agent-shell-side-dismiss' would. -The summary is left for review rather than sent, unless -`agent-shell-side-handback-submit' says otherwise." +The summary is never sent for you. It arrives as a draft you can edit, +and the compose buffer's own keys decide what happens to it: send, +queue behind the parent's running turn, or steer into it. This works +whether or not the parent is mid-turn." (interactive) (let* ((side-buffer (agent-shell-side--resolve-side)) (parent-buffer (agent-shell-side--live-buffer @@ -1104,9 +1073,6 @@ The summary is left for review rather than sent, unless (user-error "This side conversation has no parent left to report to")) (when (memq (agent-shell-status :shell-buffer side-buffer) '(busy blocked)) (user-error "The side conversation is still working; wait for it to finish")) - (when (buffer-local-value 'agent-shell-side--handback-pending side-buffer) - (user-error "Findings are already waiting for %s to finish its turn" - (buffer-name parent-buffer))) (agent-shell-side--collect-summary side-buffer parent-buffer) (agent-shell-insert :text agent-shell-side-handback-prompt :submit t diff --git a/tests/agent-shell-side-tests.el b/tests/agent-shell-side-tests.el index 529ca78..b324f87 100644 --- a/tests/agent-shell-side-tests.el +++ b/tests/agent-shell-side-tests.el @@ -357,7 +357,12 @@ The buffer looks like a started session on an agent that can fork." ;; Off by default so the suite's output stays readable. The ;; echoing itself is asserted by ;; `agent-shell-side-test-parent-status-is-echoed'. - (agent-shell-side-report-parent-status nil)) + (agent-shell-side-report-parent-status nil) + ;; Windows are the subject of a handful of tests and noise in the + ;; rest, so the default side window is opted into rather than out + ;; of. Left as the real default, every test that forks would + ;; leave a side window standing for the next one. + (agent-shell-side-display-action '(display-buffer-same-window))) (unwind-protect (progn (with-current-buffer parent @@ -807,11 +812,12 @@ the suite's own output carrying them." agent-shell-side-handback-prompt)))))) (ert-deftest agent-shell-side-test-conclude-hands-findings-to-parent () - "The summarised turn is inserted into the parent, and the side closes." + "The summary reaches the parent as an editable draft, and the side closes." (agent-shell-side-tests--with-parent (let* ((side (agent-shell-side-tests--start-side parent)) (agent-shell-side-on-dismiss 'delete) - (agent-shell-test-inserted nil)) + (agent-shell-test-inserted nil) + (agent-shell-test-viewport-calls nil)) (agent-shell-side-tests--silently (with-current-buffer side (agent-shell-side-conclude))) @@ -827,38 +833,17 @@ the suite's own output carrying them." (cl-letf (((symbol-function 'agent-shell-side--display) #'ignore)) (funcall handler '((:event . turn-complete) (:data . ((:stop-reason . "end_turn"))))))) - (let ((handback (seq-find (lambda (insertion) - (eq (map-elt insertion :shell-buffer) parent)) - agent-shell-test-inserted))) - (should handback) - (should (string-suffix-p "it pulls tokio-util" (map-elt handback :text))) - (should-not (map-elt handback :submit))) + (let ((call (car agent-shell-test-viewport-calls))) + (should call) + (should (eq (plist-get call :shell-buffer) parent)) + (should (string-suffix-p "it pulls tokio-util" (plist-get call :append))) + ;; Ready to type in, and never sent for the user. + (should (plist-get call :edit)) + (should-not (plist-get call :submit))) + ;; Nothing is pushed at the parent's prompt any more. + (should-not (agent-shell-side-tests--inserted-into parent)) (should-not (buffer-live-p side))))) -(ert-deftest agent-shell-side-test-conclude-can-submit-in-parent () - "With `agent-shell-side-handback-submit', the findings are sent, not staged." - (agent-shell-side-tests--with-parent - (let* ((side (agent-shell-side-tests--start-side parent)) - (agent-shell-side-on-dismiss 'delete) - (agent-shell-side-handback-submit t) - (agent-shell-test-inserted nil)) - (agent-shell-side-tests--silently - (with-current-buffer side - (agent-shell-side-conclude))) - (let ((handler (nth 2 (seq-find (lambda (subscription) - (and (eq (nth 0 subscription) side) - (null (nth 1 subscription)))) - agent-shell-test-subscriptions)))) - (funcall handler '((:event . agent-message-chunk) - (:data . ((:text-chunk . "done"))))) - (cl-letf (((symbol-function 'agent-shell-side--display) #'ignore)) - (funcall handler '((:event . turn-complete) - (:data . ((:stop-reason . "end_turn"))))))) - (should (map-elt (seq-find (lambda (insertion) - (eq (map-elt insertion :shell-buffer) parent)) - agent-shell-test-inserted) - :submit))))) - (ert-deftest agent-shell-side-test-conclude-keeps-side-open-when-empty () "A turn that produced no text leaves the side conversation open. @@ -1185,70 +1170,6 @@ source the real path never uses." (null (nth 1 subscription)))) agent-shell-test-subscriptions))) -(ert-deftest agent-shell-side-test-conclude-waits-for-a-busy-parent () - "A busy parent gets the findings at its prompt once its turn ends. - -Inserting at the prompt is refused while a turn runs, and would be -swallowed by the streaming output if it were not, so the findings wait -for the first event that finds the parent idle. The side conversation -stays open until then, so nothing is lost if the parent dies first. The -findings are staged, not sent: sending, queueing, or steering is the -user's call." - (agent-shell-side-tests--with-parent - (let* ((side (agent-shell-side-tests--start-side parent)) - (agent-shell-side-on-dismiss 'delete) - (agent-shell-test-inserted nil)) - (with-current-buffer parent - (setq-local agent-shell-test-status 'busy)) - (agent-shell-side-tests--conclude-with-summary side "it pulls tokio-util") - (should-not (agent-shell-side-tests--inserted-into parent)) - (should (buffer-live-p side)) - (should (buffer-local-value 'agent-shell-side--handback-pending side)) - ;; A chunk while still busy changes nothing. - (dolist (handler (agent-shell-side-tests--parent-handlers parent)) - (funcall handler '((:event . agent-message-chunk)))) - (should-not (agent-shell-side-tests--inserted-into parent)) - ;; The parent's turn ends: prompt printed, busy cleared, event emitted. - (with-current-buffer parent - (setq-local agent-shell-test-status 'ready)) - (cl-letf (((symbol-function 'agent-shell-side--display) #'ignore)) - (agent-shell-side-tests--silently - (dolist (handler (agent-shell-side-tests--parent-handlers parent)) - (funcall handler '((:event . turn-complete)))))) - (let ((staged (seq-find (lambda (insertion) - (eq (map-elt insertion :shell-buffer) parent)) - agent-shell-test-inserted))) - (should staged) - (should-not (map-elt staged :submit)) - (should (string-suffix-p "it pulls tokio-util" (map-elt staged :text)))) - (should-not (buffer-live-p side))))) - -(ert-deftest agent-shell-side-test-conclude-refuses-a-second-pending-handback () - "While findings wait for the parent, concluding again is refused." - (agent-shell-side-tests--with-parent - (let* ((side (agent-shell-side-tests--start-side parent)) - (agent-shell-test-inserted nil)) - (with-current-buffer parent - (setq-local agent-shell-test-status 'busy)) - (agent-shell-side-tests--conclude-with-summary side "findings") - (with-current-buffer side - (should-error (agent-shell-side-conclude) :type 'user-error))))) - -(ert-deftest agent-shell-side-test-conclude-pending-survives-a-closed-parent () - "A parent that closes before taking the findings leaves the side open." - (agent-shell-side-tests--with-parent - (let* ((side (agent-shell-side-tests--start-side parent)) - (agent-shell-test-inserted nil)) - (with-current-buffer parent - (setq-local agent-shell-test-status 'busy)) - (agent-shell-side-tests--conclude-with-summary side "findings") - (agent-shell-side-tests--silently - (dolist (handler (agent-shell-side-tests--parent-handlers parent)) - (funcall handler '((:event . clean-up))))) - (should-not (agent-shell-side-tests--inserted-into parent)) - (should (buffer-live-p side)) - (should-not (buffer-local-value 'agent-shell-side--handback-pending side))))) - (ert-deftest agent-shell-side-test-conclude-composes-in-the-viewport () "With viewport interaction preferred, findings go to the compose buffer. @@ -1503,54 +1424,188 @@ unrelated conversation is worse than refusing." (eq (map-elt insertion :shell-buffer) side)) agent-shell-test-inserted))))) -(ert-deftest agent-shell-side-test-conclude-stages-after-an-errored-turn () - "A parent turn that fails still takes the findings. -`error' is emitted before the shell clears its busy state, and nothing -follows it, so testing for an idle parent inline would wait forever." +;;; Where a side conversation is shown, 2026-09-06 + +(defun agent-shell-side-tests--reset-windows () + "Return the frame to a single ordinary window. + +Side windows are deleted first: `delete-other-windows' refuses to make +one the only window, so a leftover would break the next test." + (dolist (window (window-list)) + (when (window-parameter window 'window-side) + (ignore-errors (delete-window window)))) + (ignore-errors (delete-other-windows))) + +(defconst agent-shell-side-tests--side-action + '((display-buffer-in-side-window) (side . right) (window-width . 0.4)) + "The side-window action these tests exercise. + +Spelled out rather than read from the option: `agent-shell-side-tests--with-parent' +binds that option dynamically, and `default-value' on a dynamically bound +variable returns the binding, not the shipped default. That the shipped +default matches this is asserted separately, by +`agent-shell-side-test-ships-a-side-window-default'.") + +(defmacro agent-shell-side-tests--with-windows (&rest body) + "Evaluate BODY with a side-window action and a clean frame after." + (declare (indent 0)) + `(let ((agent-shell-side-display-action agent-shell-side-tests--side-action) + (agent-shell-display-action '(display-buffer-same-window))) + (unwind-protect (progn ,@body) + (agent-shell-side-tests--reset-windows)))) + +(ert-deftest agent-shell-side-test-ships-a-side-window-default () + "The option ships pointing at a window on the right." + (should (equal (eval (car (get 'agent-shell-side-display-action 'standard-value)) t) + agent-shell-side-tests--side-action))) + +(defun agent-shell-side-tests--side-windows () + "Return the frame's side windows." + (seq-filter (lambda (window) (window-parameter window 'window-side)) + (window-list))) + +(ert-deftest agent-shell-side-test-opens-beside-the-parent () + "By default a side conversation opens in a window of its own, on the right. + +The parent stays visible. That is the whole reason to have a separate +display action: a terminal has one pane and must swap, and Emacs need not." + (agent-shell-side-tests--with-parent + (agent-shell-side-tests--with-windows + (agent-shell-side-tests--reset-windows) + (set-window-buffer (selected-window) parent) + (let ((side (with-current-buffer parent (agent-shell-side)))) + (should (get-buffer-window side)) + (should (eq (window-parameter (get-buffer-window side) 'window-side) + 'right)) + ;; The point of the exercise: both are on screen. + (should (get-buffer-window parent)))))) + +(ert-deftest agent-shell-side-test-closing-deletes-the-side-window () + "The side window goes away with the side conversation. + +Handing it to the parent instead would leave the parent showing twice, +once in a strip meant for something else." + (agent-shell-side-tests--with-parent + (agent-shell-side-tests--with-windows + (agent-shell-side-tests--reset-windows) + (set-window-buffer (selected-window) parent) + (let ((side (with-current-buffer parent (agent-shell-side))) + (agent-shell-side-on-dismiss 'delete)) + (with-current-buffer side + (setq agent-shell-side--session-id-cache "side-1")) + (should (agent-shell-side-tests--side-windows)) + (agent-shell-side-tests--silently + (with-current-buffer side (agent-shell-side-dismiss))) + (should-not (agent-shell-side-tests--side-windows)) + (should (get-buffer-window parent)) + (should (equal (length (get-buffer-window-list parent)) 1)))))) + +(ert-deftest agent-shell-side-test-toggle-selects-when-both-are-visible () + "With both on screen, toggling moves point rather than rearranging windows. + +Swapping would put the side conversation in two windows at once and the +parent in none." + (agent-shell-side-tests--with-parent + (agent-shell-side-tests--with-windows + (agent-shell-side-tests--reset-windows) + (set-window-buffer (selected-window) parent) + (let ((side (with-current-buffer parent (agent-shell-side)))) + (select-window (get-buffer-window parent)) + (with-current-buffer parent (agent-shell-side-toggle)) + (should (eq (window-buffer (selected-window)) side)) + (should (equal (length (get-buffer-window-list side)) 1)) + (should (get-buffer-window parent)) + ;; And back again. + (with-current-buffer side (agent-shell-side-toggle)) + (should (eq (window-buffer (selected-window)) parent)))))) + +(ert-deftest agent-shell-side-test-same-window-action-still-swaps () + "Setting the action to the old value restores the old behaviour. + +`display-buffer-same-window' means displaying in the selected window, +which is what the swap was, so toggling needs no special case for it." + (agent-shell-side-tests--with-parent + (let ((agent-shell-side-display-action '(display-buffer-same-window))) + (unwind-protect + (progn + (agent-shell-side-tests--reset-windows) + (set-window-buffer (selected-window) parent) + (let ((side (with-current-buffer parent (agent-shell-side)))) + (should (equal (length (window-list)) 1)) + (should (eq (window-buffer (selected-window)) side)) + (with-current-buffer side (agent-shell-side-toggle)) + (should (equal (length (window-list)) 1)) + (should (eq (window-buffer (selected-window)) parent)))) + (agent-shell-side-tests--reset-windows))))) + +(ert-deftest agent-shell-side-test-parent-is-not-shown-with-the-side-action () + "The parent is an ordinary shell and is displayed like one. + +Using the side action for it would file the parent away in the strip +meant for the side conversation." + (agent-shell-side-tests--with-parent + (agent-shell-side-tests--with-windows + (agent-shell-side-tests--reset-windows) + (set-window-buffer (selected-window) parent) + (let ((side (with-current-buffer parent (agent-shell-side))) + (agent-shell-side-on-dismiss 'delete) + (elsewhere (get-buffer-create " *agent-shell side test elsewhere*"))) + (with-current-buffer side + (setq agent-shell-side--session-id-cache "side-1")) + ;; Take the parent off screen, so closing has to display it afresh. + (set-window-buffer (get-buffer-window parent) elsewhere) + (should-not (get-buffer-window parent)) + (agent-shell-side-tests--silently + (with-current-buffer side (agent-shell-side-dismiss))) + (should (get-buffer-window parent)) + (should-not (window-parameter (get-buffer-window parent) 'window-side)))))) + +;;; One handback path, whatever the viewport setting + +(ert-deftest agent-shell-side-test-conclude-composes-without-viewport-interaction () + "Findings go to a compose buffer even with viewport interaction off. + +The setting decides where the user's own prompts go; it has no business +deciding what happens to findings. A busy parent is the case that used +to differ, so that is the one asserted here." (agent-shell-side-tests--with-parent (let* ((side (agent-shell-side-tests--start-side parent)) (agent-shell-side-on-dismiss 'delete) + (agent-shell-prefer-viewport-interaction nil) (agent-shell-test-inserted nil) - (deferred nil)) + (agent-shell-test-viewport-calls nil)) (with-current-buffer parent (setq-local agent-shell-test-status 'busy)) - (agent-shell-side-tests--conclude-with-summary side "findings") - (should (buffer-local-value 'agent-shell-side--handback-pending side)) - (cl-letf (((symbol-function 'run-at-time) - (lambda (_secs _rep fn &rest _) (push fn deferred) nil))) - (funcall (agent-shell-side-tests--any-handler parent) - '((:event . error) (:data . ((:message . "boom")))))) - (should deferred) - ;; The shell clears busy right after dispatching the event. - (with-current-buffer parent - (setq-local agent-shell-test-status 'ready)) - (cl-letf (((symbol-function 'agent-shell-side--display) #'ignore)) - (agent-shell-side-tests--silently (funcall (car deferred)))) - (let ((staged (seq-find (lambda (insertion) - (eq (map-elt insertion :shell-buffer) parent)) - agent-shell-test-inserted))) - (should staged) - (should (string-suffix-p "findings" (map-elt staged :text)))) + (agent-shell-side-tests--conclude-with-summary side "it pulls tokio-util") + (let ((call (car agent-shell-test-viewport-calls))) + (should call) + (should (eq (plist-get call :shell-buffer) parent)) + (should (plist-get call :edit)) + (should (string-suffix-p "it pulls tokio-util" (plist-get call :append)))) + ;; No waiting, no pending state, and nothing at the shell prompt. + (should-not (agent-shell-side-tests--inserted-into parent)) (should-not (buffer-live-p side))))) -(ert-deftest agent-shell-side-test-lighter-reports-waiting-findings () - "While findings wait for the parent, the side's mode line says so." - (should (equal (agent-shell-side--lighter-string 'side nil t) - " Side:findings waiting")) - ;; Pending outranks the parent status that caused it. - (should (equal (agent-shell-side--lighter-string 'side 'finished t) - " Side:findings waiting")) - (should (equal (agent-shell-side--lighter-string 'side 'finished nil) - " Side:main finished")) - (agent-shell-side-tests--with-parent - (let ((side (agent-shell-side-tests--start-side parent))) - (with-current-buffer parent - (setq-local agent-shell-test-status 'busy)) - (agent-shell-side-tests--conclude-with-summary side "findings") - (with-current-buffer side - (should (equal (agent-shell-side--lighter) " Side:findings waiting")))))) +(ert-deftest agent-shell-side-test-conclude-is-the-same-either-way () + "The two viewport settings produce the same delivery." + (let ((seen nil)) + (dolist (preference '(nil t)) + (agent-shell-side-tests--with-parent + (let* ((side (agent-shell-side-tests--start-side parent)) + (agent-shell-side-on-dismiss 'delete) + (agent-shell-prefer-viewport-interaction preference) + (agent-shell-test-viewport-calls nil)) + (with-current-buffer parent + (setq-local agent-shell-test-status 'busy)) + (agent-shell-side-tests--conclude-with-summary side "findings") + (push (let ((call (car agent-shell-test-viewport-calls))) + (list (plist-get call :edit) + (plist-get call :submit) + (plist-get call :append))) + seen)))) + (should (equal (car seen) (cadr seen))) + (should (car (car seen))))) (provide 'agent-shell-side-tests) - ;;; agent-shell-side-tests.el ends here From 624b7e08fbdce76d7a099a20c14c049ddc9707c3 Mon Sep 17 00:00:00 2001 From: wliao Date: Sun, 6 Sep 2026 16:46:23 +0800 Subject: [PATCH 2/2] Address review: send summaries to the shell, keep a queued draft queued --- DESIGN.md | 52 ++++++++++++++++++++--- README.md | 13 ++++-- agent-shell-side-compat.el | 55 ++++++++++++++++++++++-- agent-shell-side.el | 45 ++++++-------------- tests/agent-shell-side-tests.el | 66 +++++++++++++++++++++++++++++ tests/live/agent-shell-side-live.el | 23 +++++++--- tests/support/agent-shell.el | 57 ++++++++++++++++++++----- 7 files changed, 249 insertions(+), 62 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 8fc91da..9bdf387 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1,7 +1,9 @@ # Design: open work for agent-shell-side -Status as of 2026-09-06. `make check` is green (104 ERT tests against stubs) -and `make live-check` passes 10 of 10 against a real claude-agent-acp 0.70. +Status as of 2026-09-06. `make check` is green (107 ERT tests against stubs). +`make live-check` last passed 10 of 10 against a real claude-agent-acp 0.70, +before the handback moved to a compose buffer; its probes were updated to +match but have not been re-run against an agent since. Four questions were open when this was written. This document records the decision for each, and what changed once they were built and run. Items 1 @@ -299,9 +301,11 @@ and what runs either side of it. clearing happens in the same call right after the event is dispatched. 4. **The README described a mode-line marker that did not exist.** The - pending flag was read only by the `conclude` guard. Fixed by building - it: the lighter now reads " Side:findings waiting", and pending - outranks parent status, which would otherwise say the same thing twice. + pending flag was read only by the `conclude` guard. It was built -- + the lighter grew a " Side:findings waiting" arm -- and then removed + again with the rest of the wait-for-idle machinery once the handback + became a compose buffer. Nothing is ever pending now, so there is + nothing for a marker to say. 5. **Resume dropped the record before the session was known to load.** `session/load` is still in flight at that point, so an agent that @@ -417,6 +421,44 @@ Removed with it: `agent-shell-side-handback-submit`, which had become a setting that could not do anything, since sending is now the compose buffer's business rather than ours. +Review of that change found four more, three of them real: + +- **`conclude` signalled from a compose buffer.** `agent-shell-insert` + dispatches on the *current* buffer, not the target: from a viewport it + routes into `agent-shell-viewport--show-buffer`, whose first two lines + reject `:submit` and `:no-focus` with "Not yet supported". So asking + the side conversation for its summary failed exactly where the compose + buffer had just become the routine place to stand. The subscription and + its 180s timer were already installed, so the user saw nothing for + three minutes and then "no summary after 180s". Both call sites now go + through `agent-shell-side-compat-send-to-shell`, which addresses the + shell buffer directly. The stub had hidden this by not dispatching at + all; making it faithful also failed an existing viewport test that had + been passing vacuously. +- **The handback downgraded a queued draft to a steer.** + `agent-shell-viewport--show-buffer` writes the compose disposition on + every call, nil included, by design. Appending findings to a prompt the + user had marked `queue` reset it, so `C-c C-c` fell through to + `agent-shell-prompt-while-busy`, whose default is `steer` -- their + queued follow-up would have been injected into the running turn. + The disposition of an in-progress draft is now read first and handed + back. +- **The new side-window arm in `--close` was dead.** + `display-buffer-in-side-window` dedicates the window, so `kill-buffer` + has already deleted it via `replace-buffer-in-windows`. Verified: the + predicate was never called, and its test passed anyway because Emacs + did the work. The arm and `agent-shell-side--side-window-p` are gone; + the test stays, since it asserts the outcome. +- Not a defect: the compose buffer is selected when the summary lands, + which the old `:no-focus` path never did. That is the point of the + change, but it is a visible difference for every user, so the README + now says so. + +The recurring lesson holds, in a third form. It was not enough to read +what an upstream function does; this one dispatches on ambient state, so +the same call means different things depending on where the user is +standing. + ## Order of work 1. Item 1's live test file, including the item 2 probe. It is the diff --git a/README.md b/README.md index 23aa8da..e9223ee 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,12 @@ buffer is also the only surface that works while the parent is mid-turn, since a busy shell cannot take text at its prompt. shell-maker appends output at the end of the buffer and would swallow it. +The draft comes to you. Summarising takes as long as the agent takes, so the +compose buffer opens and is selected whenever the summary lands, which may be +after you have moved on to something else. If the compose buffer already held an +unsent draft of your own, the findings are appended below it and your draft's +queue-or-steer choice is kept. + If the summary comes back empty, or the parent is gone, the side conversation is left open rather than closed on nothing. Customize `agent-shell-side-handback-prompt` to change what is asked for. @@ -226,10 +232,11 @@ Against claude-agent-acp 0.70, end to end: before the standing-conventions clause; see above) - `session/delete` succeeds -`make live-check` covers the rest, and passes 10 of 10 against -claude-agent-acp 0.70: +`make live-check` covers the rest. It last passed 10 of 10 against +claude-agent-acp 0.70, before the handback moved to a compose buffer: -- the handback summarises, reaches the parent's prompt staged rather than sent, +- the handback summarises, reaches the parent's compose buffer as an editable + draft rather than being sent, and closes the side conversation - a handback whose parent is killed mid-summary leaves the side conversation open rather than discarding it diff --git a/agent-shell-side-compat.el b/agent-shell-side-compat.el index edddf6d..d2d4778 100644 --- a/agent-shell-side-compat.el +++ b/agent-shell-side-compat.el @@ -41,7 +41,9 @@ (eval-when-compile (require 'cl-lib)) (declare-function agent-shell--start "agent-shell") +(declare-function agent-shell--insert-to-shell-buffer "agent-shell") (declare-function agent-shell-viewport--show-buffer "agent-shell-viewport") +(declare-function agent-shell-viewport--buffer "agent-shell-viewport") (defvar agent-shell--state) (defvar agent-shell-prefer-viewport-interaction) @@ -148,18 +150,63 @@ was issued from a viewport buffer. Mirrors `agent-shell--fork-shell-buffer'." agent-shell-side-compat--upgrade-hint)) (agent-shell-viewport--show-buffer :shell-buffer shell-buffer)) +(defun agent-shell-side-compat--compose-disposition (shell-buffer) + "Return the disposition of SHELL-BUFFER\='s in-progress compose draft. + +Nil when there is no draft to speak for. `agent-shell-viewport--show-buffer\=' +writes the disposition on every call, nil included, so appending to a +draft the user had already marked `queue\=' would silently downgrade it to +whatever `agent-shell-prompt-while-busy\=' says -- steer, by default. +Reading it first lets us hand the same answer back. + +Only an edit-mode buffer with something in it counts, matching how +`agent-shell-viewport--show-buffer\=' itself decides a draft is in +progress. An empty compose buffer keeps a stale disposition, which is +the very thing writing on every call exists to clear." + (when (fboundp 'agent-shell-viewport--buffer) + (when-let* ((viewport (ignore-errors + (agent-shell-viewport--buffer + :shell-buffer shell-buffer :existing-only t))) + ((buffer-live-p viewport))) + (with-current-buffer viewport + (and (derived-mode-p 'agent-shell-viewport-edit-mode) + (> (buffer-size) 0) + (bound-and-true-p agent-shell-viewport--compose-disposition)))))) + (defun agent-shell-side-compat-compose-in-viewport (shell-buffer text) "Open SHELL-BUFFER's viewport compose buffer with TEXT appended, to be sent. Opens in edit mode even while the shell is busy, which is the point: the compose buffer is the one surface a viewport user can write in while a -turn runs, and its send keys let them queue or steer what they wrote." +turn runs, and its send keys let them queue or steer what they wrote. + +Carries any in-progress draft's disposition through, so appending +findings to a prompt the user had already chosen to queue does not turn +it into a steer." (unless (fboundp 'agent-shell-viewport--show-buffer) (error "Missing agent-shell-viewport--show-buffer; %s" agent-shell-side-compat--upgrade-hint)) - (agent-shell-viewport--show-buffer :shell-buffer shell-buffer - :append text - :edit t)) + (agent-shell-viewport--show-buffer + :shell-buffer shell-buffer + :append text + :edit t + :disposition (agent-shell-side-compat--compose-disposition shell-buffer))) + +(defun agent-shell-side-compat-send-to-shell (shell-buffer text) + "Submit TEXT to SHELL-BUFFER without moving the user\='s focus. + +Deliberately not `agent-shell-insert\=': that dispatches on the *current* +buffer, so calling it from a viewport compose buffer routes into +`agent-shell-viewport--show-buffer\=', which signals \"Not yet supported\" +for both `:submit\=' and `:no-focus\='. The target here is always a side +conversation\='s shell buffer, never a viewport, so address it directly." + (unless (fboundp 'agent-shell--insert-to-shell-buffer) + (error "Missing agent-shell--insert-to-shell-buffer; %s" + agent-shell-side-compat--upgrade-hint)) + (agent-shell--insert-to-shell-buffer :text text + :submit t + :no-focus t + :shell-buffer shell-buffer)) (provide 'agent-shell-side-compat) diff --git a/agent-shell-side.el b/agent-shell-side.el index f07170a..a540573 100644 --- a/agent-shell-side.el +++ b/agent-shell-side.el @@ -320,10 +320,7 @@ subscription drops itself after firing." (setq sent t) (when (buffer-live-p side-buffer) (agent-shell-side--unsubscribe side-buffer token) - (agent-shell-insert :text message - :submit t - :no-focus t - :shell-buffer side-buffer)))))) + (agent-shell-side-compat-send-to-shell side-buffer message)))))) token)) @@ -802,15 +799,6 @@ before asking the user for anything." (or (map-elt parent-config :mode-line-name) "This agent"))) parent-buffer)) -(defun agent-shell-side--side-window-p (window) - "Return non-nil when WINDOW is one of the frame's side windows. - -Such a window is scenery for what it holds rather than a place to put -things: it cannot be made the only window, and handing it a different -buffer leaves that buffer stuck in a narrow strip." - (and (window-live-p window) - (window-parameter window 'window-side))) - (defun agent-shell-side--display (buffer &optional viewport) "Show side conversation BUFFER, honoring `agent-shell-side-display-action'. @@ -911,11 +899,9 @@ viewport user actually is." Disposal follows `agent-shell-side-on-dismiss'. PARENT-SHOWN means the caller has already put the parent in front of the -user, so this must not show it again. The handback sets it: under -viewport interaction the findings go into the parent's compose buffer, -and re-displaying the parent shell would re-enter the viewport with -nothing to append, which flips that compose buffer to read-only view -mode and strands the findings there unsendable." +user, so this must not show it again. The handback always sets it: the +findings land in the parent's compose buffer, and displaying the parent +shell on top would bury the draft the user is meant to send." (let* ((parent (agent-shell-side--live-buffer (buffer-local-value 'agent-shell-side--parent-buffer side-buffer))) @@ -924,16 +910,13 @@ mode and strands the findings there unsendable." (when (buffer-live-p side-buffer) (let ((window (get-buffer-window side-buffer))) (kill-buffer side-buffer) - (cond - ((not (window-live-p window)) nil) - ;; A side window belongs to the side conversation, so - ;; it goes when that does. Handing it to the parent - ;; instead would leave the parent showing twice, once - ;; in a strip it was never meant to occupy. - ((agent-shell-side--side-window-p window) - (ignore-errors (delete-window window))) - ((and parent (not parent-shown)) - (set-window-buffer window parent))))) + ;; A side window goes with the side conversation: + ;; `display-buffer-in-side-window' dedicates it, so + ;; `kill-buffer' has already deleted it here. Only an + ;; ordinary window survives to be handed the parent. + (when (and (window-live-p window) + parent (not parent-shown)) + (set-window-buffer window parent)))) (when (and parent (not parent-shown) (not (get-buffer-window parent))) (agent-shell-side--display-parent parent))))) @@ -1074,10 +1057,8 @@ whether or not the parent is mid-turn." (when (memq (agent-shell-status :shell-buffer side-buffer) '(busy blocked)) (user-error "The side conversation is still working; wait for it to finish")) (agent-shell-side--collect-summary side-buffer parent-buffer) - (agent-shell-insert :text agent-shell-side-handback-prompt - :submit t - :no-focus t - :shell-buffer side-buffer) + (agent-shell-side-compat-send-to-shell side-buffer + agent-shell-side-handback-prompt) (message "Side conversation: summarising for %s..." (buffer-name parent-buffer)))) ;;;###autoload diff --git a/tests/agent-shell-side-tests.el b/tests/agent-shell-side-tests.el index b324f87..ad0eb2f 100644 --- a/tests/agent-shell-side-tests.el +++ b/tests/agent-shell-side-tests.el @@ -1607,5 +1607,71 @@ to differ, so that is the one asserted here." (should (equal (car seen) (cadr seen))) (should (car (car seen))))) +(ert-deftest agent-shell-side-test-conclude-runs-from-a-compose-buffer () + "Concluding works from the parent's compose buffer, not just the shell. + +`agent-shell-insert' dispatches on the *current* buffer, so from a +viewport it routes into `agent-shell-viewport--show-buffer', which +signals \"Not yet supported\" for `:submit'. The summary request must +address the side conversation's shell buffer directly." + (agent-shell-side-tests--with-parent + (let* ((side (agent-shell-side-tests--start-side parent)) + (agent-shell-test-inserted nil) + (viewport (generate-new-buffer " *agent-shell side test compose*"))) + (unwind-protect + (with-current-buffer viewport + (agent-shell-viewport-edit-mode) + (setq-local agent-shell-test-viewport-shell parent) + (setq-local agent-shell-side--side-buffer side) + (agent-shell-side-tests--silently + (agent-shell-side-conclude)) + (should (agent-shell-side-tests--inserted-into side))) + (kill-buffer viewport))))) + +(ert-deftest agent-shell-side-test-handback-keeps-a-queued-draft-queued () + "Appending findings must not downgrade a queued draft into a steer. + +`agent-shell-viewport--show-buffer' writes the disposition on every +call, nil included, so passing none would drop the user's `queue' and +let `agent-shell-prompt-while-busy' steer their prompt into the running +turn instead." + (agent-shell-side-tests--with-parent + (let* ((side (agent-shell-side-tests--start-side parent)) + (agent-shell-side-on-dismiss 'delete) + (agent-shell-test-viewport-calls nil) + (viewport (generate-new-buffer " *agent-shell side test compose*"))) + (unwind-protect + (progn + (with-current-buffer viewport + (agent-shell-viewport-edit-mode) + (setq-local agent-shell-test-viewport-shell parent) + (insert "a follow-up I drafted") + (setq-local agent-shell-viewport--compose-disposition 'queue)) + (agent-shell-side-tests--conclude-with-summary side "it pulls tokio-util") + (should (eq (plist-get (car agent-shell-test-viewport-calls) :disposition) + 'queue))) + (kill-buffer viewport))))) + +(ert-deftest agent-shell-side-test-handback-does-not-inherit-a-stale-disposition () + "An empty compose buffer's leftover disposition is not carried forward. + +The disposition belongs to a draft. With no draft in progress there is +nothing to honour, and reusing the last one would apply one command's +choice to a different prompt." + (agent-shell-side-tests--with-parent + (let* ((side (agent-shell-side-tests--start-side parent)) + (agent-shell-side-on-dismiss 'delete) + (agent-shell-test-viewport-calls nil) + (viewport (generate-new-buffer " *agent-shell side test compose*"))) + (unwind-protect + (progn + (with-current-buffer viewport + (agent-shell-viewport-edit-mode) + (setq-local agent-shell-test-viewport-shell parent) + (setq-local agent-shell-viewport--compose-disposition 'steer)) + (agent-shell-side-tests--conclude-with-summary side "it pulls tokio-util") + (should-not (plist-get (car agent-shell-test-viewport-calls) :disposition))) + (kill-buffer viewport))))) + (provide 'agent-shell-side-tests) ;;; agent-shell-side-tests.el ends here diff --git a/tests/live/agent-shell-side-live.el b/tests/live/agent-shell-side-live.el index 437bf1b..e2ea56c 100644 --- a/tests/live/agent-shell-side-live.el +++ b/tests/live/agent-shell-side-live.el @@ -318,7 +318,6 @@ case up front rather than forking into nothing." "Conclude a side conversation and check its findings reach the parent." (let ((parent (agent-shell-side-live--start-parent)) (agent-shell-side-on-dismiss 'delete) - (agent-shell-side-handback-submit nil) (side nil)) (unwind-protect (progn @@ -341,12 +340,22 @@ case up front rather than forking into nothing." (format "after summarising, the side conversation should be gone; summary was: %s" (agent-shell-side-live--excerpt summary))) (agent-shell-side-live--check - "handback reaches the parent's prompt" - (with-current-buffer parent - (save-excursion - (goto-char (point-min)) - (search-forward agent-shell-side-handback-header nil t))) - "the parent buffer should hold the findings header, staged but not sent"))) + "handback reaches the parent's compose buffer" + (when-let* ((viewport (agent-shell-viewport--buffer + :shell-buffer parent :existing-only t))) + (with-current-buffer viewport + (save-excursion + (goto-char (point-min)) + (search-forward agent-shell-side-handback-header nil t)))) + "the parent's compose buffer should hold the findings header, unsent") + (agent-shell-side-live--check + "handback leaves the findings editable" + (when-let* ((viewport (agent-shell-viewport--buffer + :shell-buffer parent :existing-only t))) + (with-current-buffer viewport + (and (derived-mode-p 'agent-shell-viewport-edit-mode) + (not buffer-read-only)))) + "the findings must arrive as a draft the user can edit and send"))) (agent-shell-side-live--kill side parent)))) (defun agent-shell-side-live--probe-handback-loses-its-parent () diff --git a/tests/support/agent-shell.el b/tests/support/agent-shell.el index 4e563b2..197d8d2 100644 --- a/tests/support/agent-shell.el +++ b/tests/support/agent-shell.el @@ -51,6 +51,10 @@ (defvar agent-shell-test-viewport-calls nil "Full keyword arguments of each `agent-shell-viewport--show-buffer' call.") +(defvar-local agent-shell-viewport--compose-disposition nil + "Stub of the compose buffer's send disposition.") +(put 'agent-shell-viewport--compose-disposition 'permanent-local t) + (defvar-local agent-shell-test-viewport-shell nil "In a stub viewport buffer, the shell buffer it stands for.") (put 'agent-shell-test-viewport-shell 'permanent-local t) @@ -63,22 +67,36 @@ shell-buffer)) (buffer-list))) -(cl-defun agent-shell-viewport--show-buffer (&rest args &key shell-buffer &allow-other-keys) - "Record a viewport display, creating the stub viewport buffer on demand." +(cl-defun agent-shell-viewport--show-buffer (&rest args + &key shell-buffer submit no-focus + disposition + &allow-other-keys) + "Record a viewport display, creating the stub viewport buffer on demand. + +Refuses SUBMIT and NO-FOCUS exactly as the real one does, so a caller +that reaches a viewport when it meant to reach a shell fails here too." + (when submit + (error "Not yet supported")) + (when no-focus + (error "Not yet supported")) (push shell-buffer agent-shell-test-viewport-shown) (push args agent-shell-test-viewport-calls) - (or (agent-shell-viewport--buffer :shell-buffer shell-buffer) - (let ((buffer (generate-new-buffer " *agent-shell side test viewport*"))) - (with-current-buffer buffer - (agent-shell-viewport-edit-mode) - (setq-local agent-shell-test-viewport-shell shell-buffer)) - buffer))) + (let ((buffer (or (agent-shell-viewport--buffer :shell-buffer shell-buffer) + (let ((buffer (generate-new-buffer + " *agent-shell side test viewport*"))) + (with-current-buffer buffer + (agent-shell-viewport-edit-mode) + (setq-local agent-shell-test-viewport-shell shell-buffer)) + buffer)))) + ;; Written on every call, nil included, as the real one is. + (with-current-buffer buffer + (setq-local agent-shell-viewport--compose-disposition disposition)) + buffer)) -(cl-defun agent-shell-insert (&key text submit no-focus shell-buffer) +(cl-defun agent-shell--insert-to-shell-buffer (&key text submit no-focus shell-buffer) "Record an insertion instead of touching a shell. -Refuses when the target is busy, as the real one does -\(`agent-shell--insert-to-shell-buffer' signals \"Busy, try later\")." +Refuses when the target is busy, as the real one does." (let ((target (or shell-buffer (current-buffer)))) (when (memq (agent-shell-status :shell-buffer target) '(busy blocked)) (user-error "Busy, try later")) @@ -89,6 +107,23 @@ Refuses when the target is busy, as the real one does agent-shell-test-inserted)) nil) +(cl-defun agent-shell-insert (&key text submit no-focus shell-buffer) + "Dispatch on the current buffer, as the real one does. + +From a viewport buffer it routes to `agent-shell-viewport--show-buffer', +which signals for `:submit' and `:no-focus'. Anything that must reach a +shell buffer regardless of where the command was run has to say so." + (if (and (not (derived-mode-p 'agent-shell-mode)) + (or agent-shell-prefer-viewport-interaction + (derived-mode-p 'agent-shell-viewport-edit-mode) + (derived-mode-p 'agent-shell-viewport-view-mode))) + (agent-shell-viewport--show-buffer :append text :submit submit + :no-focus no-focus + :shell-buffer shell-buffer) + (agent-shell--insert-to-shell-buffer :text text :submit submit + :no-focus no-focus + :shell-buffer shell-buffer))) + (defun agent-shell-cwd () "Return the stubbed working directory." default-directory)