From 82158ff4898f401853cdecd7ec0244701ebf861a Mon Sep 17 00:00:00 2001 From: Ryan Schmukler Date: Sat, 6 Jun 2026 15:07:02 -0400 Subject: [PATCH] Surface waiting ask_user tool calls in remote pendingToolCalls A web/SSE client that reconnects rebuilds state via GET /api/v1/chats/:id, but a chat blocked on ask_user was invisible there: the tool call sits in :executing (not :waiting-approval) while its handler blocks on the answer, so pendingToolCalls never listed it and the client couldn't re-render the question. ask_user now mints its requestId up front, stores it in tool-call state, and passes it to the messenger so the SSE event and the persisted id match. pendingToolCalls includes :executing ask_user calls and carries requestId so a reconnecting client can answer via POST /api/v1/answer. --- CHANGELOG.md | 2 + src/eca/features/tools/ask_user.clj | 10 +++-- src/eca/remote/handlers.clj | 19 +++++---- src/eca/remote/messenger.clj | 17 ++++---- test/eca/features/tools/ask_user_test.clj | 24 +++++++++++- test/eca/remote/handlers_test.clj | 48 +++++++++++++++++++++++ test/eca/remote/messenger_test.clj | 20 ++++++++++ 7 files changed, 119 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c981d3e6..25dca2f32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Remote REST API: `GET /api/v1/chats/:id` `pendingToolCalls` now includes `ask_user` tool calls while waiting for an answer, with a `requestId` field for `POST /api/v1/answer`. + - Add OpenAI Responses API for GitHub Copilot models that require it (gpt-5.5, gpt-5.4-mini). - MCP OAuth: persist and reuse the dynamically-registered client on token refresh, so servers with non-idempotent DCR (e.g. RunLayer) refresh instead of forcing a browser re-login, and recover from expired-token tool errors automatically. diff --git a/src/eca/features/tools/ask_user.clj b/src/eca/features/tools/ask_user.clj index 3fe78b374..af9a7ea89 100644 --- a/src/eca/features/tools/ask_user.clj +++ b/src/eca/features/tools/ask_user.clj @@ -10,17 +10,21 @@ (def ^:private logger-tag "[TOOLS-USER]") (defn ^:private ask-user - [arguments {:keys [messenger chat-id tool-call-id]}] + [arguments {:keys [messenger db* chat-id tool-call-id]}] (let [question (get arguments "question") options (get arguments "options") allow-freeform (get arguments "allowFreeform" true)] (if (or (nil? question) (string/blank? question)) (tools.util/single-text-content "INVALID_ARGS: `question` is required and must not be blank." :error) - (let [params (cond-> {:chatId chat-id + (let [request-id (str (random-uuid)) + params (cond-> {:chatId chat-id :question question - :allowFreeform allow-freeform} + :allowFreeform allow-freeform + :request-id request-id} (seq options) (assoc :options options) tool-call-id (assoc :toolCallId tool-call-id))] + (when (and db* tool-call-id) + (swap! db* assoc-in [:chats chat-id :tool-calls tool-call-id :ask-question-request-id] request-id)) (try (messenger/chat-content-received messenger {:chat-id chat-id diff --git a/src/eca/remote/handlers.clj b/src/eca/remote/handlers.clj index bb9eb99fe..16ab65325 100644 --- a/src/eca/remote/handlers.clj +++ b/src/eca/remote/handlers.clj @@ -47,15 +47,19 @@ (defn ^:private camel-keys [m] (shared/map->camel-cased-map m)) -(defn ^:private waiting-approval? [[_ tc]] - (= :waiting-approval (:status tc))) +(defn ^:private pending-tool-call? [[_ tc]] + (or (= :waiting-approval (:status tc)) + ;; ask_user blocks inside its handler while :executing; surface it so + ;; reconnecting clients can render the question input. + (and (= :executing (:status tc)) + (= "ask_user" (:name tc))))) (defn ^:private pending-tool-calls - "Tool calls in :waiting-approval, shaped for the REST API so clients that - missed the toolCallRun SSE event can still render the approval card." + "Tool calls awaiting user interaction, shaped for the REST API so clients + that missed the SSE events can still render the approval card or question." [chat] (->> (:tool-calls chat) - (filter waiting-approval?) + (filter pending-tool-call?) (mapv (fn [[id tc]] (shared/assoc-some {:id id @@ -65,10 +69,11 @@ :arguments (:arguments tc) :manual-approval (boolean (:manual-approval tc))} :summary (:summary tc) - :details (:details tc)))))) + :details (:details tc) + :request-id (:ask-question-request-id tc)))))) (defn ^:private pending-approval-count [chat] - (->> (:tool-calls chat) (filter waiting-approval?) count)) + (->> (:tool-calls chat) (filter (fn [[_ tc]] (= :waiting-approval (:status tc)))) count)) (defn ^:private chat-summary [chat] (camel-keys diff --git a/src/eca/remote/messenger.clj b/src/eca/remote/messenger.clj index 4fc560097..058b775db 100644 --- a/src/eca/remote/messenger.clj +++ b/src/eca/remote/messenger.clj @@ -68,18 +68,17 @@ (editor-diagnostics [_this uri] (messenger/editor-diagnostics inner uri)) (ask-question [_this params] - ;; If there are no SSE clients, fall back to the inner messenger so - ;; JSON-RPC editor sessions keep working unchanged. Otherwise mint a - ;; requestId, register a promise, and broadcast the question to all - ;; connected SSE clients. The promise is resolved by `answer-question!` - ;; when a client posts to /api/v1/answer. + ;; No SSE clients: fall back to inner so JSON-RPC editor sessions work + ;; unchanged. Otherwise reuse the caller-supplied :request-id (set by + ;; ask_user to match the id in tool-call state) and route the answer via + ;; SSE + /api/v1/answer. (if (empty? @sse-connections*) (messenger/ask-question inner params) - (let [request-id (str (random-uuid)) - p (promise)] + (let [request-id (or (:request-id params) (str (random-uuid))) + p (promise) + wire-params (-> params (dissoc :request-id) (assoc :requestId request-id))] (swap! pending-questions* assoc request-id p) - (sse/broadcast! sse-connections* "chat:ask-question" - (->camel (assoc params :requestId request-id))) + (sse/broadcast! sse-connections* "chat:ask-question" (->camel wire-params)) p)))) (defn make-broadcast-messenger diff --git a/test/eca/features/tools/ask_user_test.clj b/test/eca/features/tools/ask_user_test.clj index cadba321c..a96493443 100644 --- a/test/eca/features/tools/ask_user_test.clj +++ b/test/eca/features/tools/ask_user_test.clj @@ -8,14 +8,15 @@ (h/reset-components-before-test) -(defn- call-ask-user [arguments & [{:keys [messenger config chat-id tool-call-id] +(defn- call-ask-user [arguments & [{:keys [messenger config chat-id tool-call-id db*] :or {tool-call-id "test-tool-call-id"}}]] ((get-in f.tools.ask-user/definitions ["ask_user" :handler]) arguments {:messenger (or messenger (h/messenger)) :config (or config (h/config)) :chat-id (or chat-id "test-chat-id") - :tool-call-id tool-call-id})) + :tool-call-id tool-call-id + :db* db*})) (deftest ask-user-option-selected-test (testing "User selects a predefined option" @@ -133,6 +134,25 @@ (future {:answer "A" :cancelled false})))}) (is (false? (:allowFreeform @captured-params)))))) +(deftest ask-user-stores-request-id-in-db-test + (testing "ask-question-request-id is written to tool-call state in db*" + (let [db* (atom {:chats {"c1" {:tool-calls {"tc-1" {:status :executing}}}}}) + captured-params (atom nil) + fake-messenger (reify messenger/IMessenger + (chat-content-received [_ _data]) + (ask-question [_ params] + (reset! captured-params params) + (future {:answer "yes" :cancelled false})))] + (call-ask-user {"question" "Ready?"} + {:messenger fake-messenger + :db* db* + :chat-id "c1" + :tool-call-id "tc-1"}) + (let [stored-id (get-in @db* [:chats "c1" :tool-calls "tc-1" :ask-question-request-id])] + (is (string? stored-id) "ask-question-request-id should be stored in db*") + (is (= stored-id (:request-id @captured-params)) + ":request-id in messenger params should match db* stored id"))))) + (deftest ask-user-summary-fn-test (testing "Summary shows Q: prefix with question" (let [summary-fn (get-in f.tools.ask-user/definitions ["ask_user" :summary-fn])] diff --git a/test/eca/remote/handlers_test.clj b/test/eca/remote/handlers_test.clj index bb428c67f..af897c4ec 100644 --- a/test/eca/remote/handlers_test.clj +++ b/test/eca/remote/handlers_test.clj @@ -138,6 +138,54 @@ body (json/parse-string (:body response) true)] (is (= [] (:pendingToolCalls body))))) + (testing "executing ask_user tool call appears in pendingToolCalls with requestId" + (swap! (h/db*) assoc-in [:chats "c1"] + {:id "c1" + :title "T" + :status :running + :tool-calls {"tc-ask" {:status :executing + :name "ask_user" + :server "eca" + :origin :native + :arguments {"question" "Proceed?" "allowFreeform" true} + :manual-approval false + :ask-question-request-id "req-123" + :summary "Q: Proceed?"} + "tc-run" {:status :executing + :name "shell_command" + :server "eca" + :origin :native + :arguments {:command "ls"}}}}) + (let [response (handlers/handle-get-chat (components) nil "c1") + body (json/parse-string (:body response) true) + pending (:pendingToolCalls body)] + (is (= 1 (count pending)) "only ask_user :executing should appear, not other :executing tool calls") + (is (= "tc-ask" (:id (first pending))) + "ask_user tool call should appear") + (is (= "ask_user" (:name (first pending)))) + (is (= false (:manualApproval (first pending)))) + (is (= "req-123" (:requestId (first pending)))) + (is (= "Q: Proceed?" (:summary (first pending)))) + (is (= "Proceed?" (get-in (first pending) [:arguments :question])) + "arguments are keywordized after JSON round-trip"))) + + (testing "executing ask_user without requestId yet omits requestId from pendingToolCalls" + (swap! (h/db*) assoc-in [:chats "c1"] + {:id "c1" + :title "T" + :status :running + :tool-calls {"tc-ask" {:status :executing + :name "ask_user" + :server "eca" + :origin :native + :arguments {"question" "Ready?"} + :manual-approval false}}}) + (let [response (handlers/handle-get-chat (components) nil "c1") + body (json/parse-string (:body response) true) + pending (:pendingToolCalls body)] + (is (= 1 (count pending))) + (is (nil? (:requestId (first pending)))))) + (testing "returns per-chat model/variant/agent overrides when set" (swap! (h/db*) assoc-in [:chats "c1"] {:id "c1" :title "T" :status :idle diff --git a/test/eca/remote/messenger_test.clj b/test/eca/remote/messenger_test.clj index 2d306e5cb..fa81651b0 100644 --- a/test/eca/remote/messenger_test.clj +++ b/test/eca/remote/messenger_test.clj @@ -86,6 +86,26 @@ "registry should be cleared after delivery")) (sse/close-all! sse-connections*)))) +(deftest ask-question-uses-caller-supplied-request-id-test + (testing "caller-supplied :request-id is used as the SSE requestId and pending-questions* key" + (let [inner (h/messenger) + sse-connections* (sse/create-connections) + broadcast-messenger (remote.messenger/make-broadcast-messenger inner sse-connections*) + os (java.io.ByteArrayOutputStream.) + _client (sse/add-client! sse-connections* os) + supplied-id "fixed-id-for-test" + p (messenger/ask-question broadcast-messenger {:chat-id "c1" :question "Q?" :request-id supplied-id})] + (Thread/sleep 100) + (let [output (.toString os "UTF-8") + pending @(:pending-questions* broadcast-messenger)] + (is (contains? pending supplied-id) "pending-questions* should be keyed by the supplied id") + (is (.contains output (str "\"requestId\":\"" supplied-id "\"")) + "SSE payload should carry the supplied requestId") + (is (not (.contains output "\"request-id\"")) ":request-id should not appear in the SSE wire payload")) + (is (true? (remote.messenger/answer-question! broadcast-messenger supplied-id "ok" false))) + (is (= {:answer "ok" :cancelled false} @p)) + (sse/close-all! sse-connections*)))) + (deftest ask-question-falls-back-to-inner-when-no-sse-clients-test (testing "ask-question delegates to inner messenger when no SSE clients are connected" (let [inner (h/messenger)