diff --git a/mobile/src/session/mobile-native-chat-rejected-draft-merge.test.ts b/mobile/src/session/mobile-native-chat-rejected-draft-merge.test.ts new file mode 100644 index 000000000000..616745f1859c --- /dev/null +++ b/mobile/src/session/mobile-native-chat-rejected-draft-merge.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest' +import { mergeRejectedDraftInto } from './mobile-native-chat-rejected-draft-merge' + +describe('mergeRejectedDraftInto', () => { + it('restores a rejected send into an empty composer verbatim', () => { + expect(mergeRejectedDraftInto({ a: '' }, 'a', 'ping')).toEqual({ a: 'ping' }) + }) + + it('restores into a composer key that does not exist yet', () => { + expect(mergeRejectedDraftInto({}, 'a', 'ping')).toEqual({ a: 'ping' }) + }) + + it('keeps both texts when the user typed while the rejection was in flight', () => { + // The rejected send has no other surviving copy; skipping it loses the message. + expect(mergeRejectedDraftInto({ a: 'newer edit' }, 'a', 'ping')).toEqual({ + a: 'ping\nnewer edit' + }) + }) + + it('does not duplicate a rejected send the composer already holds', () => { + const drafts = { a: 'ping' } + expect(mergeRejectedDraftInto(drafts, 'a', 'ping')).toBe(drafts) + }) + + it('preserves whitespace on both sides of the merge', () => { + // Trailing whitespace is load-bearing: the host writes it onto the agent's + // input line verbatim. + expect(mergeRejectedDraftInto({ a: ' spaced ' }, 'a', 'ping ')).toEqual({ + a: 'ping \n spaced ' + }) + }) + + it('leaves other tabs untouched', () => { + expect(mergeRejectedDraftInto({ a: '', b: 'other' }, 'a', 'ping')).toEqual({ + a: 'ping', + b: 'other' + }) + }) +}) diff --git a/mobile/src/session/mobile-native-chat-rejected-draft-merge.ts b/mobile/src/session/mobile-native-chat-rejected-draft-merge.ts new file mode 100644 index 000000000000..8df71b3b65fb --- /dev/null +++ b/mobile/src/session/mobile-native-chat-rejected-draft-merge.ts @@ -0,0 +1,23 @@ +/** + * Restoring the text of a send the host definitely rejected. + * + * Merge, never skip. The composer is cleared at send time, and a rejected send + * never reaches `acceptSend`, so the rejected text has no other surviving copy — + * declining to restore it because the user has since typed again loses the + * message outright. Over relay/SSH that window is seconds, not milliseconds. + * + * Ordering is by authorship: the rejected text was composed before whatever was + * typed after it, so it goes first and the newer text follows on its own line. + */ +export function mergeRejectedDraftInto( + drafts: Record, + draftKey: string, + rejected: string +): Record { + const current = drafts[draftKey] ?? '' + // An identical composer already holds the text; appending would duplicate it. + if (current === rejected) { + return drafts + } + return { ...drafts, [draftKey]: current === '' ? rejected : `${rejected}\n${current}` } +} diff --git a/mobile/src/session/use-mobile-native-chat-drafts.test.ts b/mobile/src/session/use-mobile-native-chat-drafts.test.ts index daf451e53f92..f6cec2301da6 100644 --- a/mobile/src/session/use-mobile-native-chat-drafts.test.ts +++ b/mobile/src/session/use-mobile-native-chat-drafts.test.ts @@ -125,7 +125,7 @@ describe('useMobileNativeChatDrafts', () => { expect(state?.composerText).toBe('ping') }) - it('does not clobber newer edits when restoring a rejected send', async () => { + it('merges a rejected send with newer edits instead of dropping either', async () => { await mount('a') act(() => state?.setComposerText('ping')) const origin = state?.captureSendOrigin('ping') @@ -140,7 +140,21 @@ describe('useMobileNativeChatDrafts', () => { state?.restoreRejectedDraft(origin, 'ping') } }) - expect(state?.composerText).toBe('newer edit') + // The rejected text has no other surviving copy — skipping it loses the + // message. Authoring order: the rejected send was composed first. + expect(state?.composerText).toBe('ping\nnewer edit') + }) + + it('does not duplicate a rejected send the composer already holds', async () => { + await mount('a') + act(() => state?.setComposerText('ping')) + const origin = state?.captureSendOrigin('ping') + act(() => { + if (origin) { + state?.restoreRejectedDraft(origin, 'ping') + } + }) + expect(state?.composerText).toBe('ping') }) it('restores a rejected send onto its originating tab only', async () => { diff --git a/mobile/src/session/use-mobile-native-chat-drafts.ts b/mobile/src/session/use-mobile-native-chat-drafts.ts index 55fff5b42e39..d32dfdd40e56 100644 --- a/mobile/src/session/use-mobile-native-chat-drafts.ts +++ b/mobile/src/session/use-mobile-native-chat-drafts.ts @@ -19,6 +19,7 @@ import { type MobileNativeChatPendingMessage, type MobileNativeChatSendOrigin } from './mobile-native-chat-pending-echo' +import { mergeRejectedDraftInto } from './mobile-native-chat-rejected-draft-merge' import { mobileNativeChatScopeKey } from './mobile-native-chat-scope-key' import { useMobileNativeChatLaunchDraftSeed } from './use-mobile-native-chat-launch-draft-seed' import type { MobileNativeChatLaunchDraftSeed } from './use-mobile-native-chat-launch-draft-seed' @@ -156,10 +157,7 @@ export function useMobileNativeChatDrafts(args: { }, []) const restoreRejectedDraft = useCallback((origin: MobileNativeChatSendOrigin, text: string) => { - // Why: never clobber text the user typed while the rejection was in flight. - setDrafts((previous) => - (previous[origin.draftKey] ?? '') === '' ? { ...previous, [origin.draftKey]: text } : previous - ) + setDrafts((previous) => mergeRejectedDraftInto(previous, origin.draftKey, text)) }, []) const acceptSend = useCallback( diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 3938f426b6db..06ef5a54afe6 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -79,12 +79,14 @@ import type { SleepingAgentSessionRecord } from '../../shared/agent-session-resu import { TERMINAL_INPUT_CHUNK_MAX_BYTES, TERMINAL_INPUT_MAX_BYTES, - TERMINAL_INPUT_TOO_LARGE_ERROR + TERMINAL_INPUT_TOO_LARGE_ERROR, + TERMINAL_SEND_SUBMIT_DELAY_MS } from '../../shared/terminal-input' import { MAX_QUICK_COMMANDS } from '../../shared/terminal-quick-commands' import { AGENT_PROMPT_BRACKETED_PASTE_END, AGENT_PROMPT_BRACKETED_PASTE_START, + AGENT_PROMPT_SUBMIT, AGENT_PROMPT_SUBMIT_DELAY_MS, buildAgentPromptPasteBytes } from '../../shared/agent-prompt-injection' @@ -17120,6 +17122,144 @@ describe('OrcaRuntimeService', () => { } }) + // Why: these four cover the glue window at the seam that creates it. The flow + // is ~500ms wide, roughly 10x finer than a device automation step, so it is + // only reachable deterministically here — fake timers park a send inside its + // own submit gap and start the next one by hand. + function submittedLines(writes: string[]): string[] { + return writes.join('').split(AGENT_PROMPT_SUBMIT).slice(0, -1) + } + + it('does not glue two overlapping agent prompt sends into one submitted line', async () => { + vi.useFakeTimers() + try { + const writes: string[] = [] + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + spawn: vi.fn().mockResolvedValue({ id: 'pty-bg' }), + write: (_ptyId, data) => { + writes.push(data) + return true + }, + kill: () => true, + getForegroundProcess: async () => null + }) + const { handle } = await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`) + + const first = runtime.sendTerminalAgentPrompt(handle, 'first prompt') + // Park the first send one tick short of its submit, then start the second. + await vi.advanceTimersByTimeAsync(AGENT_PROMPT_SUBMIT_DELAY_MS - 1) + const second = runtime.sendTerminalAgentPrompt(handle, 'second prompt') + await vi.advanceTimersByTimeAsync(0) + await vi.runAllTimersAsync() + await Promise.all([first, second]) + + const submitted = submittedLines(writes) + expect(submitted).toHaveLength(2) + expect(submitted[0]).toContain('first prompt') + expect(submitted[0]).not.toContain('second prompt') + expect(submitted[1]).toContain('second prompt') + } finally { + vi.useRealTimers() + } + }) + + it('does not glue two overlapping terminal.send submits into one submitted line', async () => { + vi.useFakeTimers() + try { + const writes: string[] = [] + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + spawn: vi.fn().mockResolvedValue({ id: 'pty-bg' }), + write: (_ptyId, data) => { + writes.push(data) + return true + }, + kill: () => true, + getForegroundProcess: async () => null + }) + const { handle } = await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`) + + const first = runtime.sendTerminal(handle, { text: 'first message', enter: true }) + await vi.advanceTimersByTimeAsync(TERMINAL_SEND_SUBMIT_DELAY_MS - 1) + const second = runtime.sendTerminal(handle, { text: 'second message', enter: true }) + await vi.advanceTimersByTimeAsync(0) + await vi.runAllTimersAsync() + await Promise.all([first, second]) + + expect(submittedLines(writes)).toEqual(['first message', 'second message']) + } finally { + vi.useRealTimers() + } + }) + + it('lets a bare interrupt through while a submit holds the per-PTY queue', async () => { + vi.useFakeTimers() + try { + const writes: string[] = [] + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + spawn: vi.fn().mockResolvedValue({ id: 'pty-bg' }), + write: (_ptyId, data) => { + writes.push(data) + return true + }, + kill: () => true, + getForegroundProcess: async () => null + }) + const { handle } = await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`) + + const submit = runtime.sendTerminal(handle, { text: 'long running', enter: true }) + await vi.advanceTimersByTimeAsync(1) + await runtime.sendTerminal(handle, { interrupt: true }) + + // Cancelling must not wait out the in-flight submit's gap. + expect(writes).toContain('\x03') + expect(writes).not.toContain(AGENT_PROMPT_SUBMIT) + + await vi.runAllTimersAsync() + await submit + } finally { + vi.useRealTimers() + } + }) + + it('releases the per-PTY submit queue when a send fails mid-sequence', async () => { + vi.useFakeTimers() + try { + const writes: string[] = [] + let acceptWrites = false + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + spawn: vi.fn().mockResolvedValue({ id: 'pty-bg' }), + write: (_ptyId, data) => { + if (!acceptWrites) { + return false + } + writes.push(data) + return true + }, + kill: () => true, + getForegroundProcess: async () => null + }) + const { handle } = await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`) + + const doomed = runtime.sendTerminal(handle, { text: 'doomed', enter: true }) + const doomedRejection = expect(doomed).rejects.toThrow('terminal_not_writable') + await vi.runAllTimersAsync() + await doomedRejection + + acceptWrites = true + const recovered = runtime.sendTerminal(handle, { text: 'after', enter: true }) + await vi.runAllTimersAsync() + + expect(await recovered).toMatchObject({ accepted: true }) + expect(submittedLines(writes)).toEqual(['after']) + } finally { + vi.useRealTimers() + } + }) + it('chunks large terminal.send text before provider writes', async () => { const writes: string[] = [] const runtime = new OrcaRuntimeService(store) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 6276266127e4..21617713d2e4 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -92,6 +92,7 @@ import { buildOrchestrationTaskDisplayMetadata } from '../../shared/orchestratio import { isTerminalInputTooLargeWithYield, TERMINAL_INPUT_TOO_LARGE_ERROR, + TERMINAL_SEND_SUBMIT_DELAY_MS, iterateTerminalInputChunks } from '../../shared/terminal-input' import { @@ -18346,7 +18347,59 @@ export class OrcaRuntimeService { return bestStatus ? { status: bestStatus, updatedAt: bestUpdatedAt } : null } - private async writeTerminalAction( + // Why: a submit writes its body, waits a frame for the TUI to render it, then + // writes Enter. That wait is not atomic, so two overlapping sends to one PTY + // interleave across it and their bytes land on ONE agent input line — Orca + // manufacturing the merged transcript row the pending-send matchers then have + // to recognize. Serialize submits per PTY. Same shape as `enqueueLayout` + // without the coalescing: every prompt must be delivered, none may be dropped. + private terminalSubmitQueues = new Map>() + + private enqueueTerminalSubmit(ptyId: string, run: () => Promise): Promise { + const previous = this.terminalSubmitQueues.get(ptyId) ?? Promise.resolve() + const slot = previous.then(run) + // The stored tail absorbs the rejection so one failed send cannot jam the + // queue; the caller still sees its own error via `slot`. + const tail = slot.then( + () => undefined, + () => undefined + ) + this.terminalSubmitQueues.set(ptyId, tail) + void tail.then(() => { + // Drop the entry once drained so the map does not grow across short-lived PTYs. + if (this.terminalSubmitQueues.get(ptyId) === tail) { + this.terminalSubmitQueues.delete(ptyId) + } + }) + return slot + } + + private writeTerminalAction( + ptyId: string, + action: { text?: string; enter?: boolean; interrupt?: boolean }, + payload: string, + options: { + beforeWrite?: (ptyId: string) => void | Promise + reserveWrite?: (ptyId: string) => void + afterWrite?: (ptyId: string) => void | Promise + suffixFailureError?: string + } = {} + ): Promise { + const isSubmit = + typeof action.text === 'string' && action.text.length > 0 && Boolean(action.enter) + // Why: only a body→delay→suffix sequence owns a gap to interleave across. + // Single-write actions — a bare interrupt, a bare Enter, keystrokes with no + // submit — stay unqueued so Ctrl+C and live typing are never held behind a + // queued send. + if (!isSubmit) { + return this.runTerminalAction(ptyId, action, payload, options) + } + return this.enqueueTerminalSubmit(ptyId, () => + this.runTerminalAction(ptyId, action, payload, options) + ) + } + + private async runTerminalAction( ptyId: string, action: { text?: string; enter?: boolean; interrupt?: boolean }, payload: string, @@ -18367,7 +18420,7 @@ export class OrcaRuntimeService { if (hasSuffix) { const suffix = (action.enter ? '\r' : '') + (action.interrupt ? '\x03' : '') if (hasText) { - await new Promise((resolve) => setTimeout(resolve, 500)) + await new Promise((resolve) => setTimeout(resolve, TERMINAL_SEND_SUBMIT_DELAY_MS)) } try { await options.beforeWrite?.(ptyId) @@ -18424,7 +18477,21 @@ export class OrcaRuntimeService { } } - private async writeTerminalAgentPrompt( + private writeTerminalAgentPrompt( + ptyId: string, + pastePayload: string, + options: { + beforeWrite?: (ptyId: string) => void | Promise + suffixFailureError?: string + } = {} + ): Promise { + // Always a submit: paste body, render gate (or fixed delay), then Enter. + return this.enqueueTerminalSubmit(ptyId, () => + this.writeAgentPromptFrame(ptyId, pastePayload, options) + ) + } + + private async writeAgentPromptFrame( ptyId: string, pastePayload: string, options: { diff --git a/src/shared/terminal-input.ts b/src/shared/terminal-input.ts index bdaa410d04b6..e1651622154f 100644 --- a/src/shared/terminal-input.ts +++ b/src/shared/terminal-input.ts @@ -6,6 +6,10 @@ import { import { getUtf8ChunkEndIndex } from './utf8-byte-limits' export const TERMINAL_INPUT_CHUNK_MAX_BYTES = 16 * 1024 +// Why: a text send that also submits separates body from Enter so the TUI has a +// frame to render the paste first. That gap is what two overlapping sends to one +// PTY would otherwise interleave across. +export const TERMINAL_SEND_SUBMIT_DELAY_MS = 500 export const TERMINAL_INPUT_MAX_BYTES = 16 * 1024 * 1024 export const TERMINAL_INPUT_TOO_LARGE_ERROR = 'Terminal input is too large for a safe terminal send.'