From fa222277ad719f58b68579ac8bd07ab91471e7da Mon Sep 17 00:00:00 2001 From: tomekpanek Date: Thu, 4 Jun 2026 18:09:11 +0200 Subject: [PATCH] fix(chat): make agent-prompted modals actionable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Approval / Clarification / Sudo / Secret modals rendered a header with no buttons, then click-outside closed them silently and the chat locked: STOP button red, Send dead, recovered only by refresh. Two layered bugs: 1. src/js/chat/gateway.js called showModal with {body, confirmText, cancelText, onConfirm, onCancel} — an older shape. The current modal.js takes {message, inputs, buttons} and returns a Promise. Only ``title`` survived the mismatch, so buttons[] stayed empty and neither callback fired on close. Ported the four helpers to await the Promise; null result (click-outside / Cancel) sends a negative response so the agent unblocks via chat.done/chat.error. 2. lib/tui-gateway-bridge.js sent ``approval.respond`` without session_id; the gateway's _sess() rejected it with 4001 "session not found" (surfaced as ⚠️ in chat). It also sent {approve, command} where the gateway reads {choice: "approve"|"deny"}, and ``clarify.respond`` sent {text, choice} where the gateway reads {answer}. Bridge now tracks _internalSid (set on every prompt.submit) and translates each respond into the keys the gateway expects. Regression-covered by test/chat-approval-modal.test.js. --- dist/index.html | 2 +- lib/tui-gateway-bridge.js | 16 ++++- src/js/chat/gateway.js | 107 +++++++++++++++++-------------- test/chat-approval-modal.test.js | 60 +++++++++++++++++ 4 files changed, 133 insertions(+), 52 deletions(-) create mode 100644 test/chat-approval-modal.test.js diff --git a/dist/index.html b/dist/index.html index 2393878..1425a30 100644 --- a/dist/index.html +++ b/dist/index.html @@ -13,7 +13,7 @@ - + diff --git a/lib/tui-gateway-bridge.js b/lib/tui-gateway-bridge.js index e47fde7..14c1ebc 100644 --- a/lib/tui-gateway-bridge.js +++ b/lib/tui-gateway-bridge.js @@ -35,6 +35,7 @@ class TuiGatewayBridge { this._readyReject = null; this._sidMap = new Map(); // internal TUI sid → canonical DB session_key this._canonicalSid = null; // active canonical session ID + this._internalSid = null; // active internal TUI sid, sent on approval.respond } // ── Lifecycle ────────────────────────────────────────────────────── @@ -424,6 +425,7 @@ class TuiGatewayBridge { } } + this._internalSid = internalSid; await this.request('prompt.submit', { session_id: internalSid, text: message }); return { session_id: canonicalSid }; } @@ -482,16 +484,26 @@ class TuiGatewayBridge { } } console.log(`[TUI:${this.profile}] chat.send → prompt.submit(${internalSid})`); + this._internalSid = internalSid; await this.request('prompt.submit', { session_id: internalSid, text: message }); return { session_id: this._canonicalSid }; } + // Translate WS protocol into the keys the gateway's JSON-RPC actually reads: + // - clarify.respond / sudo.respond / secret.respond → _respond() reads + // params["answer"] / "password" / "value" + // - approval.respond → _sess() requires session_id; choice is "approve"|"deny" async respondClarify(request_id, text, choice) { - return this.request('clarify.respond', { request_id, text, choice }); + const answer = (choice ?? text ?? ''); + return this.request('clarify.respond', { request_id, answer }); } async respondApproval(approve, command) { - return this.request('approval.respond', { approve, command }); + return this.request('approval.respond', { + session_id: this._internalSid, + choice: approve ? 'approve' : 'deny', + command, + }); } async respondSudo(request_id, password) { diff --git a/src/js/chat/gateway.js b/src/js/chat/gateway.js index bf13d36..b946935 100644 --- a/src/js/chat/gateway.js +++ b/src/js/chat/gateway.js @@ -1,7 +1,7 @@ import { state, t, wsClient } from '../core/state.js';; import { addToolCallCard, finalizeToolCard, highlightCodeBlocks, renderChatContent, swapOptimisticMessage, updateStreamContent, updateToolProgress } from './cli.js'; import { handleArtifact, playChatComplete, refreshChatSidebar, reloadCurrentSessionMessages, sendChatMessage, updateChatHeader, updateQueueBadge, updateToolCountUI } from './core.js'; -import { closeModal, showModal } from '../components/modal.js'; +import { showModal } from '../components/modal.js'; import { escapeHtml } from '../core/utils.js'; async function sendViaGatewayAPI(text, profile, sessionId, contentDiv, messagesDiv, startTime) { @@ -412,76 +412,85 @@ function handleSubagentEvent(type, payload) { } } -function showClarifyModal(question, choices, requestId) { +// The four agent-prompted modals use showModal's {message, inputs, buttons} +// shape from src/js/components/modal.js. A null result (click-outside or +// Cancel) sends a negative response so the agent unblocks and the chat +// lock clears naturally via chat.done/chat.error. +async function showClarifyModal(question, choices, requestId) { if (!choices || choices.length === 0) { - // Open-ended: text input - showModal({ + const result = await showModal({ title: 'Clarification Needed', - body: `

${escapeHtml(question)}

`, - confirmText: 'Submit', - onConfirm: () => { - const input = document.getElementById('clarify-input'); - if (input) wsClient.clarifyRespond(requestId, input.value); - } + message: `

${escapeHtml(question)}

`, + inputs: [{ type: 'text', placeholder: 'Your answer...' }], + buttons: [ + { text: 'Cancel', value: null }, + { text: 'Submit', primary: true, value: 'ok' }, + ], }); + if (!result || result.action === null) { + wsClient.clarifyRespond(requestId, ''); + } else { + wsClient.clarifyRespond(requestId, result.inputs?.[0] || ''); + } } else { - // Multiple choice - const buttons = choices.map(c => - `` - ).join(''); - showModal({ + const result = await showModal({ title: 'Clarification Needed', - body: `

${escapeHtml(question)}

${buttons}
`, - showCancel: false, - confirmText: null, - onRender: () => { - document.querySelectorAll('.modal-choice-btn').forEach(btn => { - btn.onclick = () => { - wsClient.clarifyRespond(requestId, null, btn.dataset.choice); - closeModal(); - }; - }); - } + message: `

${escapeHtml(question)}

`, + buttons: choices.map(c => ({ text: c, value: c })), }); + if (!result) { + wsClient.clarifyRespond(requestId, null, null); + } else { + wsClient.clarifyRespond(requestId, null, result.action); + } } } -function showApprovalModal(command, description) { - showModal({ +async function showApprovalModal(command, description) { + const result = await showModal({ title: 'Approval Required', - body: ` + message: `

${escapeHtml(description || 'The agent wants to run a command:')}

${escapeHtml(command)}
`, - confirmText: '✅ Approve', - cancelText: '❌ Deny', - onConfirm: () => wsClient.approvalRespond(true, command), - onCancel: () => wsClient.approvalRespond(false, command), + buttons: [ + { text: '❌ Deny', value: false }, + { text: '✅ Approve', primary: true, value: true }, + ], }); + if (result?.action === true) { + wsClient.approvalRespond(true, command); + } else { + wsClient.approvalRespond(false, command); + } } -function showSudoModal(requestId) { - showModal({ +async function showSudoModal(requestId) { + const result = await showModal({ title: 'Sudo Password Required', - body: `

The agent needs sudo privileges. Enter your password:

`, - confirmText: 'Submit', - onConfirm: () => { - const input = document.getElementById('sudo-input'); - if (input) wsClient.sudoRespond(requestId, input.value); - } + message: '

The agent needs sudo privileges. Enter your password:

', + inputs: [{ type: 'password', placeholder: 'Password' }], + buttons: [ + { text: 'Cancel', value: null }, + { text: 'Submit', primary: true, value: 'ok' }, + ], }); + const password = (result && result.action !== null) ? (result.inputs?.[0] || '') : ''; + wsClient.sudoRespond(requestId, password); } -function showSecretModal(envVar, prompt, requestId) { - showModal({ +async function showSecretModal(envVar, prompt, requestId) { + const result = await showModal({ title: `Secret Required: ${escapeHtml(envVar)}`, - body: `

${escapeHtml(prompt || `Enter value for ${envVar}:`)}

`, - confirmText: 'Submit', - onConfirm: () => { - const input = document.getElementById('secret-input'); - if (input) wsClient.secretRespond(requestId, input.value); - } + message: `

${escapeHtml(prompt || `Enter value for ${envVar}:`)}

`, + inputs: [{ type: 'password', placeholder: 'Value' }], + buttons: [ + { text: 'Cancel', value: null }, + { text: 'Submit', primary: true, value: 'ok' }, + ], }); + const value = (result && result.action !== null) ? (result.inputs?.[0] || '') : ''; + wsClient.secretRespond(requestId, value); } function ensureThinkingPanel(messagesDiv) { diff --git a/test/chat-approval-modal.test.js b/test/chat-approval-modal.test.js new file mode 100644 index 0000000..f96f032 --- /dev/null +++ b/test/chat-approval-modal.test.js @@ -0,0 +1,60 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const test = require('node:test'); + +const ROOT = path.resolve(__dirname, '..'); + +function read(relPath) { + return fs.readFileSync(path.join(ROOT, relPath), 'utf8'); +} + +function extractFunction(source, name) { + const re = new RegExp(`(?:async\\s+)?function\\s+${name}\\s*\\([^)]*\\)\\s*\\{`, 'm'); + const match = re.exec(source); + assert.ok(match, `missing function ${name}`); + let i = match.index + match[0].length - 1; + let depth = 0; + for (; i < source.length; i += 1) { + const ch = source[i]; + if (ch === '{') depth += 1; + if (ch === '}') { + depth -= 1; + if (depth === 0) return source.slice(match.index, i + 1); + } + } + throw new Error(`unterminated function ${name}`); +} + +test('Approval Required modal is actionable with approve and deny controls', () => { + const gatewaySource = read('src/js/chat/gateway.js'); + const modalSource = read('src/js/components/modal.js'); + const approval = extractFunction(gatewaySource, 'showApprovalModal'); + const showModal = extractFunction(modalSource, 'showModal'); + + const approvalUsesModernButtons = + /buttons\s*:\s*\[/.test(approval) + && /approvalRespond\(\s*true\s*,\s*command\s*\)/.test(approval) + && /approvalRespond\(\s*false\s*,\s*command\s*\)/.test(approval); + + const showModalSupportsLegacyApprovalApi = + /body/.test(showModal) + && /confirmText/.test(showModal) + && /cancelText/.test(showModal) + && /onConfirm/.test(showModal) + && /onCancel/.test(showModal); + + assert.ok( + approvalUsesModernButtons || showModalSupportsLegacyApprovalApi, + 'showApprovalModal currently passes body/confirmText/cancelText/onConfirm/onCancel; showModal must support that API or approval must use buttons[] with approve/deny actions', + ); +}); + +test('Approval Required modal includes command and description in its body', () => { + const gatewaySource = read('src/js/chat/gateway.js'); + const approval = extractFunction(gatewaySource, 'showApprovalModal'); + + assert.match(approval, /description/); + assert.match(approval, /command/); + assert.match(approval, /escapeHtml\(command\)/); +});