From 62c37f40d89deecaef89b55de77fa4fcf0091813 Mon Sep 17 00:00:00 2001 From: Ame <123734885+luokerenx4@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:15:51 +0800 Subject: [PATCH] fix(workspaces): preserve resumed sessions and support text answers --- docs/ui-interaction-and-motion.md | 9 +++ docs/web-conversation-surface.md | 11 ++- src/webui/routes/workspaces.spec.ts | 7 +- src/webui/routes/workspaces.ts | 8 ++- src/workspaces/web-session-host.spec.ts | 69 +++++++++++++++++++ src/workspaces/web-session-host.ts | 12 ++-- src/workspaces/web-session/acp-transport.ts | 2 +- .../web-session/codex-app-server-transport.ts | 16 +++-- src/workspaces/web-session/model.ts | 4 +- src/workspaces/web-session/transport.ts | 2 +- .../ConversationRequestCard.spec.tsx | 29 ++++++++ .../conversation/ConversationRequestCard.tsx | 27 ++++++-- .../workspace/WebSessionView.spec.tsx | 15 +++- .../components/workspace/WebSessionView.tsx | 3 + ui/src/components/workspace/api.ts | 7 +- .../workspace/useWebConversation.ts | 4 +- ui/src/demo/README.md | 4 ++ ui/src/demo/handlers/workspaces-web.spec.ts | 13 ++++ ui/src/demo/handlers/workspaces.ts | 19 ++++- 19 files changed, 232 insertions(+), 29 deletions(-) create mode 100644 ui/src/components/conversation/ConversationRequestCard.spec.tsx diff --git a/docs/ui-interaction-and-motion.md b/docs/ui-interaction-and-motion.md index 7c2386a36..0b80e6090 100644 --- a/docs/ui-interaction-and-motion.md +++ b/docs/ui-interaction-and-motion.md @@ -501,3 +501,12 @@ For motion changes: Motion should be judged in the running UI. A class name or screenshot alone cannot prove timing, continuity, or pointer feedback. + +Web question cards keep the existing composer status placement. Text-capable +questions show a labeled shared Textarea and an explicit Send answer button; +offered options stay available above it. Secret questions use a masked field. +The layout stacks vertically at narrow widths and does not steal focus. +Permission cards remain option-only. A failed submission retains the draft, +while a new request ID mounts a fresh card so answers do not leak between +questions. This is owned by the shared ConversationRequestCard, not a +runtime-specific presenter. diff --git a/docs/web-conversation-surface.md b/docs/web-conversation-surface.md index 0e7ab3651..ce3bb21b8 100644 --- a/docs/web-conversation-surface.md +++ b/docs/web-conversation-surface.md @@ -37,6 +37,10 @@ approval or cannot reopen an exact recorded conversation. | `claude-stream-json` | `claude` | `-p --input-format stream-json --output-format stream-json --include-partial-messages --permission-prompt-tool stdio` | `control_request` `can_use_tool`; answered with allow/deny | `--session-id ` chosen by the adapter | | `codex-app-server` | `codex` | `codex app-server --listen stdio://` with MCP registration, `approvalPolicy: on-request`, `sandbox: workspace-write` | `item/commandExecution/requestApproval`, `item/fileChange/requestApproval` (answered with a `decision` enum), `item/permissions/requestApproval` (answered with the granted `permissions` profile + `scope`), `item/tool/requestUserInput` | `thread/start`; resume via `thread/resume` | +If ACP does not advertise `loadSession`, opening an existing Session fails with +terminal guidance and preserves its native ID. Never replace its transcript +with `session/new`; only a separate fresh Session may allocate a new ID. + The Web surface never resumes "last": it reopens the exact recorded native id or starts a fresh conversation the transport reports back, so the Session's `resumeId` binds to one native transcript exactly as PTY discovery does. @@ -87,8 +91,11 @@ components, labels) use "Web". User-facing copy says "Web", never "WebPi". running headless turn, disposes a PTY on the same record, starts the host. - `GET /web?revision=` — snapshot or `{ unchanged: true }`. - `POST /web/prompt`, `POST /web/abort` — turn control. -- `POST /web/respond { requestId, optionId }` — answers one request; the - transport validates the option id and fails with `web_respond_failed`. +- `POST /web/respond { requestId, optionId, text? }` — answers one request; the + transport validates the option id and fails with `web_respond_failed`. For a + question with `allowText`, send `optionId: ""` and nonblank `text` to answer + freely. Permission requests reject text answers. Secret questions use a masked + field. Cancelling a turn clears the entire pending question sequence. Switching a running Web Session to the TUI stops the Web process first; the reverse disposes the PTY. Exactly one process may own a Session record. diff --git a/src/webui/routes/workspaces.spec.ts b/src/webui/routes/workspaces.spec.ts index 0c8700c38..8a94913b2 100644 --- a/src/webui/routes/workspaces.spec.ts +++ b/src/webui/routes/workspaces.spec.ts @@ -1736,9 +1736,14 @@ describe('Web surface routes', () => { const { app, web } = buildWeb(); const result = await post(app, `/ws-1/sessions/${TOKEN}/web/respond`, { requestId: 'acp-7', optionId: 'allow_once' }); expect(result.status).toBe(200); - expect(web.respond).toHaveBeenCalledWith(TOKEN, 'acp-7', 'allow_once'); + expect(web.respond).toHaveBeenCalledWith(TOKEN, 'acp-7', 'allow_once', undefined); const bad = await post(app, `/ws-1/sessions/${TOKEN}/web/respond`, { requestId: 'acp-7' }); expect(bad.status).toBe(400); + const answer = await post(app, `/ws-1/sessions/${TOKEN}/web/respond`, { requestId: 'q1', optionId: '', text: 'Alice' }); + expect(answer.status).toBe(200); + expect(web.respond).toHaveBeenCalledWith(TOKEN, 'q1', '', 'Alice'); + const invalid = await post(app, `/ws-1/sessions/${TOKEN}/web/respond`, { requestId: 'q1', optionId: '', text: 123 }); + expect(invalid.status).toBe(400); }); it('returns a tiny unchanged response when the browser already has the revision', async () => { diff --git a/src/webui/routes/workspaces.ts b/src/webui/routes/workspaces.ts index bed601f60..fd3cd36b4 100644 --- a/src/webui/routes/workspaces.ts +++ b/src/webui/routes/workspaces.ts @@ -2581,7 +2581,8 @@ export function createWorkspaceRoutes( }); // Answer a runtime permission/question request with one of the options the - // runtime itself offered. The transport validates the option id. + // runtime itself offered, or free text for a question. The host and transport + // validate the answer against the pending request. app.post('/:id/sessions/:sid/web/respond', async (c) => { const ctx = webSessionContext(c); if (!ctx) return c.json({ error: 'not_found' }, 404); @@ -2589,11 +2590,12 @@ export function createWorkspaceRoutes( const fields = body && typeof body === 'object' ? body as Record : {}; const requestId = fields['requestId']; const optionId = fields['optionId']; - if (typeof requestId !== 'string' || !requestId || typeof optionId !== 'string') { + const text = fields['text']; + if (typeof requestId !== 'string' || !requestId || typeof optionId !== 'string' || (text !== undefined && typeof text !== 'string')) { return c.json({ error: 'bad_request', message: 'requestId and optionId are required' }, 400); } try { - const snapshot = await svc.web.respond(ctx.token, requestId, optionId); + const snapshot = await svc.web.respond(ctx.token, requestId, optionId, text as string | undefined); await svc.sessionRegistry.update(ctx.id, ctx.token, { lastActiveAt: new Date().toISOString() }); return c.json({ ok: true, snapshot }); } catch (err) { diff --git a/src/workspaces/web-session-host.spec.ts b/src/workspaces/web-session-host.spec.ts index 38295d8c0..3e62a2d38 100644 --- a/src/workspaces/web-session-host.spec.ts +++ b/src/workspaces/web-session-host.spec.ts @@ -294,6 +294,22 @@ describe('WebSessionHost with the acp transport', () => { expect(process.received.find((c) => c['id'] === 'srv-1')).toMatchObject({ result: { outcome: { outcome: 'selected', optionId: 'allow-once' } } }) }) + it('refuses unsupported restore without creating or rebinding a native session', async () => { + const process = acpProcess({ loadSession: false }) + const bind = vi.fn() + const host = new WebSessionHost(logger, { onNativeSessionId: bind }, () => process as never) + await expect(host.start({ ...acpInput, nativeSessionId: 'ses_old' })).rejects.toThrow(/terminal/) + expect(process.received.some((frame) => frame['method'] === 'session/new')).toBe(false) + expect(bind).not.toHaveBeenCalled() + expect(host.has('record-1')).toBe(false) + }) + + it('still creates fresh sessions when ACP cannot load history', async () => { + const host = new WebSessionHost(logger, {}, () => acpProcess({ loadSession: false }) as never) + expect((await host.start(acpInput)).nativeSessionId).toBe('ses_new') + await host.stopAll() + }) + it('reloads a known session and replays its history', async () => { const host = new WebSessionHost(logger, {}, () => acpProcess() as never) const started = await host.start({ ...acpInput, nativeSessionId: 'ses_old' }) @@ -554,3 +570,56 @@ describe('WebSessionHost with the codex-app-server transport', () => { expect(texts(snapshot).at(-1)).toBe('notice:Turn interrupted.') }) }) + + +describe('Codex question answers', () => { + async function setup() { + const process = codexProcess() + const host = new WebSessionHost(logger, {}, () => process as never) + await host.start(input({ agent: 'codex', wire: 'codex-app-server', command: ['codex', 'app-server'] })) + process.line({ id: 'questions', method: 'item/tool/requestUserInput', params: { questions: [ + { id: 'name', header: 'Project name', question: 'What name?', options: null }, + { id: 'style', header: 'Style', question: 'Which style?', options: [{ label: 'Simple' }], isOther: true }, + ] } }) + await settle() + return { process, host } + } + + it('round-trips free text and then a selected option', async () => { + const { process, host } = await setup() + const first = host.get('record-1')!.requests[0]! + expect(first).toMatchObject({ allowText: true, options: [] }) + await expect(host.respond('record-1', first.id, '', ' ')).rejects.toThrow(/text/) + expect(host.get('record-1')!.requests).toHaveLength(1) + await host.respond('record-1', first.id, '', 'Alice research') + await settle() + await host.respond('record-1', host.get('record-1')!.requests[0]!.id, 'Simple') + await settle() + expect(process.received.find((frame) => frame['id'] === 'questions')).toMatchObject({ result: { answers: { + name: { answers: ['Alice research'] }, style: { answers: ['Simple'] }, + } } }) + await host.stopAll() + }) + + it('does not enqueue later questions when the turn is cancelled', async () => { + const { process, host } = await setup() + process.line({ method: 'turn/completed', params: { turn: { id: 't', status: 'interrupted' } } }) + await settle() + expect(host.get('record-1')!.requests).toEqual([]) + expect(host.get('record-1')!.phase).toBe('idle') + expect(process.received.find((frame) => frame['id'] === 'questions')).toMatchObject({ result: { answers: {} } }) + await host.stopAll() + }) + + it('rejects text answers to permission requests without consuming the request', async () => { + const process = codexProcess({ permissions: true }) + const host = new WebSessionHost(logger, {}, () => process as never) + await host.start(input({ agent: 'codex', wire: 'codex-app-server', command: ['codex', 'app-server'] })) + await host.prompt('record-1', 'test') + await settle() + const request = host.get('record-1')!.requests[0]! + await expect(host.respond('record-1', request.id, 'grant', 'yes')).rejects.toThrow(/text/) + expect(host.get('record-1')!.requests).toHaveLength(1) + await host.stopAll() + }) +}) diff --git a/src/workspaces/web-session-host.ts b/src/workspaces/web-session-host.ts index 8c1b7209d..12005b299 100644 --- a/src/workspaces/web-session-host.ts +++ b/src/workspaces/web-session-host.ts @@ -128,9 +128,9 @@ export class WebSessionHost { return session.snapshot() } - async respond(recordId: string, requestId: string, optionId: string): Promise { + async respond(recordId: string, requestId: string, optionId: string, text?: string): Promise { const session = this.require(recordId) - await session.respond(requestId, optionId) + await session.respond(requestId, optionId, text) return session.snapshot() } @@ -231,9 +231,13 @@ class LiveWebSession { return this.transport.abort() } - respond(requestId: string, optionId: string): Promise { + respond(requestId: string, optionId: string, text?: string): Promise { this.assertLive() - return this.transport.respond(requestId, optionId) + const request = this.state.requests.find((entry) => entry.id === requestId) + if (text !== undefined && (request?.kind !== 'question' || !request.allowText || optionId !== '' || !text.trim())) { + return Promise.reject(new Error('This request does not accept this text answer')) + } + return this.transport.respond(requestId, optionId, text) } async stop(reason: string): Promise { diff --git a/src/workspaces/web-session/acp-transport.ts b/src/workspaces/web-session/acp-transport.ts index d8e9e7e0b..5c2b73ea4 100644 --- a/src/workspaces/web-session/acp-transport.ts +++ b/src/workspaces/web-session/acp-transport.ts @@ -60,7 +60,7 @@ export class AcpTransport implements WebSessionTransport { this.builder.endTurn() } else { if (this.sessionId) { - this.builder.notice(`${this.ctx.input.agent} cannot reload session ${this.sessionId} over ACP; a new native session was started.`) + throw new Error(`${this.ctx.input.agent} cannot reopen this conversation over ACP. Open the existing Session in the terminal, or create a separate new Session.`) } const created = await this.peer.request('session/new', params) const id = isJsonObject(created) ? stringOrNull(created['sessionId']) : null diff --git a/src/workspaces/web-session/codex-app-server-transport.ts b/src/workspaces/web-session/codex-app-server-transport.ts index 786ba1754..6dc5185f4 100644 --- a/src/workspaces/web-session/codex-app-server-transport.ts +++ b/src/workspaces/web-session/codex-app-server-transport.ts @@ -42,6 +42,7 @@ export class CodexAppServerTransport implements WebSessionTransport { private threadId: string | null private turnId: string | null = null private requestSeq = 0 + private cancellationEpoch = 0 constructor(private readonly ctx: WebTransportContext) { this.threadId = ctx.input.nativeSessionId ?? null @@ -103,16 +104,18 @@ export class CodexAppServerTransport implements WebSessionTransport { await this.peer.request('turn/interrupt', { threadId: this.threadId, turnId: this.turnId }) } - async respond(requestId: string, optionId: string): Promise { + async respond(requestId: string, optionId: string, text?: string): Promise { const request = this.ctx.state.requests.find((r) => r.id === requestId) const pending = this.approvals.get(requestId) if (!request || !pending) throw new Error(`no pending request ${requestId}`) - if (!request.options.some((option) => option.id === optionId)) { + const textAnswer = text !== undefined && request.kind === 'question' && request.allowText && optionId === '' && text.trim() + if (text !== undefined && !textAnswer) throw new Error('Invalid text answer') + if (!textAnswer && !request.options.some((option) => option.id === optionId)) { throw new Error(`option ${optionId} is not offered by request ${requestId}`) } this.approvals.delete(requestId) this.ctx.state.removeRequest(requestId) - pending.answer(pending.respondWith(optionId)) + pending.answer(pending.respondWith(textAnswer ? text! : optionId)) } dispose(): void { @@ -359,6 +362,7 @@ export class CodexAppServerTransport implements WebSessionTransport { private async userInput(params: JsonObject): Promise { const questions = Array.isArray(params['questions']) ? params['questions'].filter(isJsonObject) : [] const answers: Record = {} + const epoch = this.cancellationEpoch for (const question of questions) { const questionId = stringOrNull(question['id']) ?? `q${Object.keys(answers).length}` const options = Array.isArray(question['options']) @@ -372,8 +376,11 @@ export class CodexAppServerTransport implements WebSessionTransport { kind: 'question', title: stringOrNull(question['header']) ?? 'Codex has a question', description: stringOrNull(question['question']) ?? '', - options: options.length > 0 ? options : [{ id: '', label: 'Continue without an answer', tone: 'neutral' }], + options, + allowText: true, + secret: question['isSecret'] === true, }, (optionId) => optionId, '') + if (epoch !== this.cancellationEpoch) return { answers: {} } answers[questionId] = { answers: typeof choice === 'string' && choice ? [choice] : [] } } return { answers } @@ -393,6 +400,7 @@ export class CodexAppServerTransport implements WebSessionTransport { } private cancelApprovals(): void { + this.cancellationEpoch += 1 for (const [id, pending] of this.approvals) { this.approvals.delete(id) this.ctx.state.removeRequest(id) diff --git a/src/workspaces/web-session/model.ts b/src/workspaces/web-session/model.ts index 07f9f80c2..e560e0e59 100644 --- a/src/workspaces/web-session/model.ts +++ b/src/workspaces/web-session/model.ts @@ -46,11 +46,13 @@ export interface WebRequestOption { /** * A question the runtime cannot answer on its own: a tool permission, a file * change approval, or a free-form user question. The browser presents the - * options verbatim and answers with one `optionId`. + * options verbatim and answers with one `optionId`, or text when allowed. */ export interface WebPermissionRequest { readonly id: string readonly kind: 'permission' | 'question' + readonly allowText?: boolean + readonly secret?: boolean readonly title: string readonly description?: string readonly tool?: { readonly name: string; readonly input: unknown } diff --git a/src/workspaces/web-session/transport.ts b/src/workspaces/web-session/transport.ts index 5f83e2816..6f6e1ea6e 100644 --- a/src/workspaces/web-session/transport.ts +++ b/src/workspaces/web-session/transport.ts @@ -143,7 +143,7 @@ export interface WebSessionTransport { prompt(message: string): Promise abort(): Promise /** Answer one outstanding request with the chosen option id. */ - respond(requestId: string, optionId: string): Promise + respond(requestId: string, optionId: string, text?: string): Promise /** Polite shutdown before the host closes stdin and signals the process. */ dispose?(): void } diff --git a/ui/src/components/conversation/ConversationRequestCard.spec.tsx b/ui/src/components/conversation/ConversationRequestCard.spec.tsx new file mode 100644 index 000000000..94f351dec --- /dev/null +++ b/ui/src/components/conversation/ConversationRequestCard.spec.tsx @@ -0,0 +1,29 @@ +// @vitest-environment jsdom +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { afterEach, expect, it, vi } from 'vitest' +import { ConversationRequestCard } from './ConversationRequestCard' + +afterEach(cleanup) +const question = { id: 'q1', kind: 'question' as const, title: 'Project name', allowText: true, options: [] } + +it('retains the draft after an error and permits a retry', async () => { + const respond = vi.fn().mockRejectedValueOnce(new Error('Connection lost')).mockResolvedValue(undefined) + render() + const field = screen.getByLabelText('Your answer') as HTMLTextAreaElement + fireEvent.change(field, { target: { value: 'Alice research' } }) + fireEvent.click(screen.getByRole('button', { name: 'Send answer' })) + await screen.findByRole('alert') + expect(field.value).toBe('Alice research') + fireEvent.click(screen.getByRole('button', { name: 'Send answer' })) + await waitFor(() => expect(respond).toHaveBeenCalledTimes(2)) + expect(respond).toHaveBeenLastCalledWith('q1', '', 'Alice research') +}) + +it('masks secret input and does not expose text fields on permissions', () => { + const { unmount } = render() + expect(screen.getByLabelText('Your answer').getAttribute('type')).toBe('password') + unmount() + render() + expect(screen.queryByLabelText('Your answer')).toBeNull() + expect(screen.getByRole('button', { name: 'Allow' })).toBeTruthy() +}) diff --git a/ui/src/components/conversation/ConversationRequestCard.tsx b/ui/src/components/conversation/ConversationRequestCard.tsx index cda2fc5fa..aee11d1f4 100644 --- a/ui/src/components/conversation/ConversationRequestCard.tsx +++ b/ui/src/components/conversation/ConversationRequestCard.tsx @@ -1,6 +1,7 @@ import { useState, type ReactElement } from 'react' import { CircleHelp, LoaderCircle, ShieldQuestion } from 'lucide-react' import { Button } from '../ui/button' +import { Textarea } from '../ui/textarea' export interface ConversationRequestOption { readonly id: string @@ -11,6 +12,8 @@ export interface ConversationRequestOption { export interface ConversationRequest { readonly id: string readonly kind: 'permission' | 'question' + readonly allowText?: boolean + readonly secret?: boolean readonly title: string readonly description?: string /** Pre-rendered tool detail; the adapter decides how the input is summarized. */ @@ -22,7 +25,7 @@ export interface ConversationRequestCardProps { readonly request: ConversationRequest /** How many further requests wait behind this one. */ readonly queued: number - readonly respond: (requestId: string, optionId: string) => Promise + readonly respond: (requestId: string, optionId: string, text?: string) => Promise } /** @@ -34,13 +37,14 @@ export interface ConversationRequestCardProps { export function ConversationRequestCard({ request, queued, respond }: ConversationRequestCardProps): ReactElement { const [pendingOption, setPendingOption] = useState(null) const [error, setError] = useState(null) + const [answer, setAnswer] = useState('') const permission = request.kind === 'permission' - async function choose(optionId: string) { - if (pendingOption) return + async function choose(optionId: string, text?: string) { + if (pendingOption !== null) return setPendingOption(optionId) setError(null) - try { await respond(request.id, optionId) } + try { await respond(request.id, optionId, text) } catch (cause) { setError(cause instanceof Error ? cause.message : String(cause)) } finally { setPendingOption(null) } } @@ -91,6 +95,21 @@ export function ConversationRequestCard({ request, queued, respond }: Conversati ))} + {!permission && request.allowText &&
{ + event.preventDefault() + if (answer.trim()) void choose('', answer) + }}> + + {request.secret + ? setAnswer(event.target.value)} /> + :