diff --git a/mobile/src/session/mobile-native-chat-permission-send.test.ts b/mobile/src/session/mobile-native-chat-permission-send.test.ts index 1b5956831..253f8c8b8 100644 --- a/mobile/src/session/mobile-native-chat-permission-send.test.ts +++ b/mobile/src/session/mobile-native-chat-permission-send.test.ts @@ -13,6 +13,11 @@ import { markMobileNativeChatInputStale, resetMobileNativeChatStaleInputForTests } from './mobile-native-chat-stale-input' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite, + resetMobileNativeChatTerminalWritesForTests +} from './mobile-native-chat-terminal-write-lock' describe('sendMobileNativeChatPermissionResponse', () => { it('writes an approval as raw bytes without appending Return', async () => { @@ -64,6 +69,7 @@ describe('useMobileNativeChatPermissionSend', () => { beforeEach(() => { globalThis.IS_REACT_ACT_ENVIRONMENT = true resetMobileNativeChatStaleInputForTests() + resetMobileNativeChatTerminalWritesForTests() }) afterEach(() => { @@ -102,4 +108,42 @@ describe('useMobileNativeChatPermissionSend', () => { expect(sendRequest.mock.calls[0]?.[1]).toMatchObject({ text: '1', enter: false }) expect(isMobileNativeChatInputStale('terminal')).toBe(true) }) + + it('rejects a choice while another composed write holds the terminal, then recovers', async () => { + const onSendError = vi.fn() + const sendRequest = vi.fn().mockResolvedValue({ + ok: true, + result: { send: { handle: 'terminal', accepted: true, bytesWritten: 1 } } + }) + function Harness(): null { + respond = useMobileNativeChatPermissionSend({ + client: { sendRequest } as unknown as RpcClient, + enabled: true, + handleRef: { current: 'terminal' }, + deviceTokenRef: { current: null }, + onSendError + }) + return null + } + act(() => { + renderer = create(createElement(Harness)) + }) + + // An image paste sequence is mid-flight into the same PTY: the choice + // keystroke must not interleave into it. + expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(true) + await act(async () => { + await expect(respond?.('1')).resolves.toBe(false) + }) + expect(sendRequest).not.toHaveBeenCalled() + expect(onSendError).toHaveBeenCalledWith('Response not sent') + + releaseMobileNativeChatTerminalWrite('terminal') + await act(async () => { + await expect(respond?.('1')).resolves.toBe(true) + }) + // The choice released its own hold on the way out. + expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(true) + releaseMobileNativeChatTerminalWrite('terminal') + }) }) diff --git a/mobile/src/session/mobile-native-chat-permission-send.ts b/mobile/src/session/mobile-native-chat-permission-send.ts index b06ff88fe..7f58b0ab4 100644 --- a/mobile/src/session/mobile-native-chat-permission-send.ts +++ b/mobile/src/session/mobile-native-chat-permission-send.ts @@ -4,6 +4,10 @@ import { sendMobileNativeChatMessageWithOutcome, type MobileNativeChatSendOutcome } from './mobile-native-chat-send' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite +} from './mobile-native-chat-terminal-write-lock' export function sendMobileNativeChatPermissionResponse(args: { client: RpcClient @@ -36,15 +40,26 @@ export function useMobileNativeChatPermissionSend(args: { args.onSendError('Response not sent (disconnected)') return false } + // A choice keystroke must not interleave into a mid-flight composed write + // (image paste, paced answer) on the same PTY. + if (!acquireMobileNativeChatTerminalWrite(terminal)) { + args.onSendError('Response not sent') + return false + } // No stale-input heal here (unlike the text/ask sends): a choice is an // `enter: false` key for an active overlay that swallows the clear, so it // would consume the marker still protecting the next real message. - const outcome = await sendMobileNativeChatPermissionResponse({ - client: args.client, - terminal, - deviceToken: args.deviceTokenRef.current, - text - }) + let outcome: MobileNativeChatSendOutcome + try { + outcome = await sendMobileNativeChatPermissionResponse({ + client: args.client, + terminal, + deviceToken: args.deviceTokenRef.current, + text + }) + } finally { + releaseMobileNativeChatTerminalWrite(terminal) + } if (outcome === 'unknown') { // Why: the response may have been delivered (ack lost / path cutover) — // a definite "not sent" would invite a double answer. diff --git a/mobile/src/session/mobile-native-chat-terminal-write-lock.test.ts b/mobile/src/session/mobile-native-chat-terminal-write-lock.test.ts new file mode 100644 index 000000000..c15ce3c82 --- /dev/null +++ b/mobile/src/session/mobile-native-chat-terminal-write-lock.test.ts @@ -0,0 +1,20 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite, + resetMobileNativeChatTerminalWritesForTests +} from './mobile-native-chat-terminal-write-lock' + +describe('mobile-native-chat-terminal-write-lock', () => { + afterEach(resetMobileNativeChatTerminalWritesForTests) + + it('allows composed writes on different terminals to proceed concurrently', () => { + expect(acquireMobileNativeChatTerminalWrite('terminal-a')).toBe(true) + + expect(acquireMobileNativeChatTerminalWrite('terminal-b')).toBe(true) + expect(acquireMobileNativeChatTerminalWrite('terminal-a')).toBe(false) + + releaseMobileNativeChatTerminalWrite('terminal-a') + releaseMobileNativeChatTerminalWrite('terminal-b') + }) +}) diff --git a/mobile/src/session/mobile-native-chat-terminal-write-lock.ts b/mobile/src/session/mobile-native-chat-terminal-write-lock.ts new file mode 100644 index 000000000..5624a384e --- /dev/null +++ b/mobile/src/session/mobile-native-chat-terminal-write-lock.ts @@ -0,0 +1,25 @@ +// Serializes composed native-chat write sequences (clear/paste/settle/submit, +// paced answer keystrokes) per HOST terminal. Two concurrent sequences into one +// PTY interleave their bytes; a second sender must be rejected up front, not +// woven in. Module scope for the same reason as the stale-input marker: the +// terminal outlives any one screen, and independent hooks share the same PTY. +const writeInFlightTerminals = new Set() + +/** Claim the terminal for one composed write sequence. False = another + * sequence is mid-flight; the caller must reject its send. */ +export function acquireMobileNativeChatTerminalWrite(terminal: string): boolean { + if (writeInFlightTerminals.has(terminal)) { + return false + } + writeInFlightTerminals.add(terminal) + return true +} + +export function releaseMobileNativeChatTerminalWrite(terminal: string): void { + writeInFlightTerminals.delete(terminal) +} + +/** Test-only: module scope outlives a single test's hooks. */ +export function resetMobileNativeChatTerminalWritesForTests(): void { + writeInFlightTerminals.clear() +} diff --git a/mobile/src/session/use-mobile-native-chat-answer-send.test.ts b/mobile/src/session/use-mobile-native-chat-answer-send.test.ts index 8f2288414..b296ea485 100644 --- a/mobile/src/session/use-mobile-native-chat-answer-send.test.ts +++ b/mobile/src/session/use-mobile-native-chat-answer-send.test.ts @@ -11,7 +11,13 @@ import { markMobileNativeChatInputStale, resetMobileNativeChatStaleInputForTests } from './mobile-native-chat-stale-input' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite, + resetMobileNativeChatTerminalWritesForTests +} from './mobile-native-chat-terminal-write-lock' import { useMobileNativeChatAnswerSend } from './use-mobile-native-chat-answer-send' +import { useNativeChatAcceptedAction } from './use-native-chat-action-outcomes' type AnswerSend = ReturnType @@ -40,17 +46,24 @@ describe('useMobileNativeChatAnswerSend', () => { let mountedClient: RpcClient | null = null let mountedOnSendError: ((message: string) => void) | null = null let mountedAgent: AgentType = 'claude' + // The route sends through useNativeChatAcceptedAction, whose accepted callback + // retires the shared send-error banner (use-mobile-native-chat-controller.ts). + let acceptedAnswerAsk: AnswerSend['answerAsk'] | null = null + let onAccepted = vi.fn() beforeEach(() => { + onAccepted = vi.fn() vi.useFakeTimers() globalThis.IS_REACT_ACT_ENVIRONMENT = true resetMobileNativeChatStaleInputForTests() + resetMobileNativeChatTerminalWritesForTests() }) afterEach(() => { act(() => renderer?.unmount()) renderer = null answerSend = null + acceptedAnswerAsk = null mountedClient = null mountedOnSendError = null mountedAgent = 'claude' @@ -68,6 +81,7 @@ describe('useMobileNativeChatAnswerSend', () => { streamIdentity: 'host\0worktree\0tab\0session', onSendError: mountedOnSendError! }) + acceptedAnswerAsk = useNativeChatAcceptedAction(answerSend.answerAsk, onAccepted) return null } @@ -396,4 +410,517 @@ describe('useMobileNativeChatAnswerSend', () => { await expect(result).resolves.toBe(false) expect(sendRequest).toHaveBeenCalledTimes(1) }) + + it('rejects an answer while another composed write holds the terminal', async () => { + const onSendError = vi.fn() + const sendRequest = vi.fn().mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + // An image paste sequence is mid-flight into the same PTY. + expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(true) + await expect(answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }])).resolves.toBe(false) + expect(sendRequest).not.toHaveBeenCalled() + expect(onSendError).toHaveBeenCalledWith('Answer not sent') + + // Once that sequence releases, answers flow again. + releaseMobileNativeChatTerminalWrite('terminal') + await expect(answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }])).resolves.toBe(true) + }) + + it('answers again on the same handle after an earlier answer already landed', async () => { + const onSendError = vi.fn() + const sendRequest = vi.fn().mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + await expect(answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }])).resolves.toBe(true) + // A landed answer resolves its turn FALSE — correct for a queued successor, + // fatal if the turn outlives the chain. Leaving it parked in the slot fences + // every later answer on this handle for the life of the hook, not just an + // overlapping one, so the ask card dies after its first use. + await expect(answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }])).resolves.toBe(true) + expect(sendRequest).toHaveBeenCalledTimes(2) + expect(onSendError).not.toHaveBeenCalled() + }) + + it('fences a superseding answer after the cancelled chain moved the selector', async () => { + const sendRequest = vi.fn().mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + const prompt: AskPrompt = { + questions: [ + { question: 'q1', multiSelect: false, options: [{ label: 'A' }, { label: 'B' }] }, + { question: 'q2', multiSelect: false, options: [{ label: 'C' }, { label: 'D' }] } + ] + } + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(prompt, [{ indices: [0] }, { indices: [0] }]) + }) + // The first digit already advanced Claude to q2. Replaying a from-q1 key + // plan now would answer the wrong question. + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + }) + await act(async () => vi.runAllTimersAsync()) + + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + // Both chains unwound: the terminal is free for the next composed write. + expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(true) + releaseMobileNativeChatTerminalWrite('terminal') + }) + + it('does not write a successor after the prior in-flight key is accepted', async () => { + let resolveFirst: (response: unknown) => void = () => undefined + const sendRequest = vi + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = resolve + }) + ) + .mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + const prompt: AskPrompt = { + questions: [ + { question: 'q1', multiSelect: false, options: [{ label: 'A' }, { label: 'B' }] }, + { question: 'q2', multiSelect: false, options: [{ label: 'C' }, { label: 'D' }] } + ] + } + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(prompt, [{ indices: [0] }, { indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + + // The successor is queued behind the in-flight key, not racing it. + expect(sendRequest).toHaveBeenCalledTimes(1) + await act(async () => { + resolveFirst(acceptedResponse()) + await Promise.resolve() + }) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + }) + + it('lets a queued successor continue after the prior key is definitely rejected', async () => { + let resolveFirst: (response: unknown) => void = () => undefined + const sendRequest = vi + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = resolve + }) + ) + .mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + resolveFirst({ + ...acceptedResponse(), + result: { send: { accepted: false } } + }) + await Promise.resolve() + }) + + // Nothing landed, so the selector never moved — the successor is safe. + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(true) + expect(sendRequest).toHaveBeenCalledTimes(2) + }) + + it('does not write a successor after the prior delivery becomes ambiguous', async () => { + let rejectFirst: (error: Error) => void = () => undefined + const sendRequest = vi + .fn() + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectFirst = reject + }) + ) + .mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + rejectFirst(markRpcDeliveryUnknown(new Error('Connection closed'))) + await Promise.resolve() + }) + + // The key may have landed; a blind successor could double-step the selector. + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + }) + + it('tells the user when a queued answer is fenced instead of dropping it silently', async () => { + const onSendError = vi.fn() + let rejectFirst: (error: Error) => void = () => undefined + const sendRequest = vi + .fn() + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectFirst = reject + }) + ) + .mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + rejectFirst(markRpcDeliveryUnknown(new Error('Connection closed'))) + await Promise.resolve() + }) + + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + // The card re-enables on a false result, so an unreported fence looks exactly + // like a dead button. The superseded chain stays quiet — the newest answer + // owns the error surface, and it is the one the user is waiting on. + expect(onSendError).toHaveBeenCalledTimes(1) + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + }) + + it('tells the user when a dropped lease fences a queued answer whose predecessor landed', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + // A transport blip, not a user cancel: unlike Stop and ask-cancel, the lost + // input lease writes no Escape, so the card stays up with Submit re-enabled. + await setEnabled(false) + await act(async () => { + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + + expect(sendRequest).toHaveBeenCalledTimes(1) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + // The first option key LANDED, so the live selector already moved. Reporting + // nothing invites a retry that double-steps it. + expect(onSendError).toHaveBeenCalledTimes(1) + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + }) + + it('keeps the terminal locked while a queued successor writes', async () => { + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + expect(sendRequest).toHaveBeenCalledTimes(1) + + // Nothing landed, so the successor is cleared to write. + await act(async () => { + settle[0]!({ ...acceptedResponse(), result: { send: { accepted: false } } }) + await Promise.resolve() + }) + expect(sendRequest).toHaveBeenCalledTimes(2) + // The successor's key is on the wire. The superseded chain unwinding behind it + // must NOT free the terminal, or an image paste interleaves into this sequence. + expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(false) + + await act(async () => { + settle[1]!(acceptedResponse()) + await Promise.resolve() + }) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(true) + expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(true) + releaseMobileNativeChatTerminalWrite('terminal') + }) + + it('does not retire the fence banner when the superseded answer lands', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + // The healthy path: the first answer LANDS, which is also the case that fences + // hardest — its key moved the live selector. + await act(async () => { + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + // A superseded chain reporting success would clear the banner it just raised — + // the accepted hook runs after the fence, so the user would see nothing at all. + expect(onAccepted).not.toHaveBeenCalled() + expect(sendRequest).toHaveBeenCalledTimes(1) + }) + + it('does not retire the fence banner when a superseded pasted answer lands', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError, 'grok') + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + + // The pasted shape commits with Enter, so a superseded chain is doubly unsafe + // to report as accepted — the answer it committed is not the one on screen. + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(onAccepted).not.toHaveBeenCalled() + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + }) + + it('reports a landed answer that Stop cancelled with no successor waiting', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + await act(async () => { + first = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + // Stop, an ask cancel, and a dropped input lease all bump the generation with + // NO successor chain behind them. + await act(async () => { + answerSend?.cancelPending() + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + + // The key is on the PTY and nobody else owns the surface. Calling that a + // non-send leaves the card up and silent (fail() never runs on an accepted + // write), and the retry double-steps the selector this key already moved. + await expect(first).resolves.toBe(true) + expect(onAccepted).toHaveBeenCalledTimes(1) + expect(onSendError).not.toHaveBeenCalled() + }) + + it('reports a landed pasted answer that Stop cancelled with no successor', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError, 'grok') + + let first: Promise | undefined + await act(async () => { + first = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + answerSend?.cancelPending() + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + + // The pasted shape already committed with Enter, so suppressing the success + // strands an answered card the user is invited to submit a second time. + await expect(first).resolves.toBe(true) + expect(onAccepted).toHaveBeenCalledTimes(1) + expect(onSendError).not.toHaveBeenCalled() + }) + + it('fences a third answer behind an already-fenced successor, reporting once', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + let second: Promise | undefined + let third: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + third = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + expect(sendRequest).toHaveBeenCalledTimes(1) + + await act(async () => { + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + await expect(third).resolves.toBe(false) + // The middle chain sent nothing, so only the verdict it INHERITED can stop the + // third from replaying a from-scratch plan onto the advanced selector. + expect(sendRequest).toHaveBeenCalledTimes(1) + // Only the newest chain owns the error surface. + expect(onSendError).toHaveBeenCalledTimes(1) + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + }) + + it('queues a late third answer behind the successor already on the wire', async () => { + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + let first: Promise | undefined + let second: Promise | undefined + let third: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + // Nothing landed, so the successor is cleared and puts its own key on the wire. + await act(async () => { + settle[0]!({ ...acceptedResponse(), result: { send: { accepted: false } } }) + await Promise.resolve() + }) + expect(sendRequest).toHaveBeenCalledTimes(2) + + await act(async () => { + third = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + await Promise.resolve() + }) + // The first chain unwound while the second was mid-write: it must not have + // dropped the second's turn, or this one writes into the same PTY concurrently. + expect(sendRequest).toHaveBeenCalledTimes(2) + + await act(async () => { + settle[1]!(acceptedResponse()) + await Promise.resolve() + }) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + await expect(third).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(2) + }) }) diff --git a/mobile/src/session/use-mobile-native-chat-answer-send.ts b/mobile/src/session/use-mobile-native-chat-answer-send.ts index 11a3a0307..b893f7ae0 100644 --- a/mobile/src/session/use-mobile-native-chat-answer-send.ts +++ b/mobile/src/session/use-mobile-native-chat-answer-send.ts @@ -14,6 +14,10 @@ import { sendMobileNativeChatMessageWithOutcome } from './mobile-native-chat-send' import { healMobileNativeChatStaleInput } from './mobile-native-chat-stale-input' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite +} from './mobile-native-chat-terminal-write-lock' import { resolveNativeChatTranscriptAgent, shouldStepNativeChatAskAnswer @@ -72,6 +76,12 @@ export function useMobileNativeChatAnswerSend(args: { const generationRef = useRef(0) const activeRouteRef = useRef({ client, enabled, sessionId, streamIdentity }) activeRouteRef.current = { client, enabled, sessionId, streamIdentity } + // Per-terminal count of this hook's chains sharing one write-lock hold: a + // superseding answer inherits the cancelled chain's hold (it re-enters before + // the old chain unwinds), and only the last chain out releases the lock. + const writeHoldsRef = useRef(new Map()) + // Successors wait for the prior RPC and inherit any delivery ambiguity. + const writeTurnsRef = useRef(new Map>()) const delaysRef = useRef< Set<{ timer: ReturnType; resolve: (completed: boolean) => void }> >(new Set()) @@ -103,125 +113,191 @@ export function useMobileNativeChatAnswerSend(args: { if (!hasAskAnswer(prompt, selections)) { return false } + // One composed write sequence per terminal: an answer landing mid-flight + // in an image paste (or vice versa) would interleave bytes into the PTY. + // A superseding answer shares the cancelled chain's hold on this terminal + // (that chain has not unwound to its release yet). + const holds = writeHoldsRef.current + const heldCount = holds.get(handle) ?? 0 + if (heldCount === 0 && !acquireMobileNativeChatTerminalWrite(handle)) { + onSendError('Answer not sent') + return false + } + holds.set(handle, heldCount + 1) + const previousTurn = writeTurnsRef.current.get(handle) ?? Promise.resolve(true) + let finishTurn: (safeToContinue: boolean) => void = () => undefined + const turn = new Promise((resolve) => { + finishTurn = resolve + }) + writeTurnsRef.current.set(handle, turn) // A new answer supersedes any still-pending keystroke writes. cancelPending() const generation = generationRef.current let sawUnknownOutcome = false let sawAcceptedGroup = false - // One budget for the whole answer instead of a fresh timeout per keystroke - // group, which let an N-group selector hold the card for N × the send timeout. - // It bounds transport time only: each deliberate pacing wait is credited back - // below, so a long multi-question answer still gets a full budget to write in. - let deadline = openMobileNativeChatSendBudget() - const sendTerminal = async (body: string, enter: boolean): Promise => { - const activeRoute = activeRouteRef.current - if ( - !activeRoute.enabled || - activeRoute.client !== client || - activeRoute.sessionId !== sessionId || - activeRoute.streamIdentity !== streamIdentity || - handleRef.current !== handle - ) { - return false - } - const outcome = await sendMobileNativeChatMessageWithOutcome({ - client, - terminal: handle, - text: body, - enter, - deadline, - ...(deviceTokenRef.current - ? { mobileClient: { id: deviceTokenRef.current, type: 'mobile' } } - : {}) - }) - if (outcome === 'unknown') { - sawUnknownOutcome = true - } - if (outcome === 'accepted') { - sawAcceptedGroup = true - } - return outcome === 'accepted' - } - const wait = (ms: number): Promise => - new Promise((resolve) => { - const delay = { - timer: setTimeout(() => { - delaysRef.current.delete(delay) - resolve(generationRef.current === generation) - }, ms), - resolve - } - delaysRef.current.add(delay) - }) - const fail = (): false => { - if (generationRef.current === generation) { - // Why: keystrokes that may have landed (ack lost / path cutover) must - // not read as a definite failure — a blind resend could double-step - // the selector. An earlier group that WAS accepted is the same hazard - // in definite form: a multi-question answer whose shared budget ran out - // mid-sequence left the remote selector half-stepped, and telling the - // user nothing was sent invites a retry on top of the advanced state. - onSendError( - sawAcceptedGroup - ? 'Answer partly sent — check chat before retrying' - : sawUnknownOutcome - ? 'Answer unconfirmed — check chat before retrying' - : 'Answer not sent' - ) - } - return false - } - // Grok commits pasted labels; Claude and Codex need their selector-specific - // keystrokes paced so each step renders before the next lands. - if (!shouldStepNativeChatAskAnswer(agentRef.current)) { - // This shape pastes the label into the composer and commits it, so an - // orphaned image paste would be submitted along with the answer (#10228). - // The selector shapes below deliberately skip the heal: their keys are - // `enter: false` for an active overlay, and a single-select answer is a - // bare option digit that cannot submit the line at all, so clearing there - // would consume the marker still protecting the next real message. - // Desktop splits it identically — use-native-chat-interactive-send.ts - // routes only the pasted-label shape through the clearing sender. - if ( - !(await healMobileNativeChatStaleInput({ - client, - terminal: handle, - deviceToken: deviceTokenRef.current, - deadline - })) - ) { - if (generationRef.current === generation) { - onSendError('Answer not sent') + let predecessorSafe = true + try { + predecessorSafe = await previousTurn + if (!predecessorSafe) { + // Fenced. Report it: the card re-enables on a false result, so silence + // here is indistinguishable from a dead button. "Check chat" rather than + // a bare "not sent" because the PREVIOUS answer's keys may have landed. + // Gate on the turn slot, not the generation: a dropped input lease bumps + // the generation without writing the Escape that Stop and ask-cancel do, + // so the card is still up and silence there strands an advanced selector. + if (writeTurnsRef.current.get(handle) === turn) { + onSendError('Answer not sent — check chat before retrying') } return false } + // Superseded by a newer answer, which owns the error surface from here. if (generationRef.current !== generation) { return false } - return (await sendTerminal(formatAskAnswer(prompt, selections), true)) || fail() - } - const groups = - resolveNativeChatTranscriptAgent(agentRef.current) === 'codex' - ? buildCodexAskAnswerKeys(prompt, selections) - : buildAskAnswerKeys(prompt, selections) - for (let index = 0; index < groups.length; index += 1) { - if (generationRef.current !== generation) { - return false - } - const group = groups[index]! - const body = 'raw' in group ? group.raw : sanitizeAskFreeText(group.text) - if (!(await sendTerminal(body, false))) { - return fail() - } - if (index < groups.length - 1) { - if (!(await wait(MOBILE_NATIVE_CHAT_QUESTION_STEP_MS))) { + // One budget for the whole answer instead of a fresh timeout per keystroke + // group, which let an N-group selector hold the card for N × the send timeout. + // It bounds transport time only: each deliberate pacing wait is credited back + // below, so a long multi-question answer still gets a full budget to write in. + let deadline = openMobileNativeChatSendBudget() + const sendTerminal = async (body: string, enter: boolean): Promise => { + const activeRoute = activeRouteRef.current + if ( + !activeRoute.enabled || + activeRoute.client !== client || + activeRoute.sessionId !== sessionId || + activeRoute.streamIdentity !== streamIdentity || + handleRef.current !== handle + ) { return false } - // Pacing is deliberate, not transport latency — don't charge it to the budget. - deadline += MOBILE_NATIVE_CHAT_QUESTION_STEP_MS + const outcome = await sendMobileNativeChatMessageWithOutcome({ + client, + terminal: handle, + text: body, + enter, + deadline, + ...(deviceTokenRef.current + ? { mobileClient: { id: deviceTokenRef.current, type: 'mobile' } } + : {}) + }) + if (outcome === 'unknown') { + sawUnknownOutcome = true + } + if (outcome === 'accepted') { + sawAcceptedGroup = true + } + return outcome === 'accepted' + } + const wait = (ms: number): Promise => { + // Already superseded: don't hold the successor for a full pacing step + // waiting on a timer whose only job is to report the cancellation. + if (generationRef.current !== generation) { + return Promise.resolve(false) + } + return new Promise((resolve) => { + const delay = { + timer: setTimeout(() => { + delaysRef.current.delete(delay) + resolve(generationRef.current === generation) + }, ms), + resolve + } + delaysRef.current.add(delay) + }) + } + const fail = (): false => { + if (generationRef.current === generation) { + // Why: keystrokes that may have landed (ack lost / path cutover) must + // not read as a definite failure — a blind resend could double-step + // the selector. An earlier group that WAS accepted is the same hazard + // in definite form: a multi-question answer whose shared budget ran out + // mid-sequence left the remote selector half-stepped, and telling the + // user nothing was sent invites a retry on top of the advanced state. + onSendError( + sawAcceptedGroup + ? 'Answer partly sent — check chat before retrying' + : sawUnknownOutcome + ? 'Answer unconfirmed — check chat before retrying' + : 'Answer not sent' + ) + } + return false + } + // Grok commits pasted labels; Claude and Codex need their selector-specific + // keystrokes paced so each step renders before the next lands. + if (!shouldStepNativeChatAskAnswer(agentRef.current)) { + // This shape pastes the label into the composer and commits it, so an + // orphaned image paste would be submitted along with the answer (#10228). + // The selector shapes below deliberately skip the heal: their keys are + // `enter: false` for an active overlay, and a single-select answer is a + // bare option digit that cannot submit the line at all, so clearing there + // would consume the marker still protecting the next real message. + // Desktop splits it identically — use-native-chat-interactive-send.ts + // routes only the pasted-label shape through the clearing sender. + if ( + !(await healMobileNativeChatStaleInput({ + client, + terminal: handle, + deviceToken: deviceTokenRef.current, + deadline + })) + ) { + if (generationRef.current === generation) { + onSendError('Answer not sent') + } + return false + } + if (generationRef.current !== generation) { + return false + } + // A chain a successor took over from must not report success either: an + // accepted answer retires the shared send-error banner, wiping the + // successor's fence. Test the turn slot, not the generation counter — + // Stop, ask-cancel and a dropped lease all bump the generation with no + // successor, and there a landed answer IS a success. + const sent = (await sendTerminal(formatAskAnswer(prompt, selections), true)) || fail() + return sent && writeTurnsRef.current.get(handle) === turn + } + const groups = + resolveNativeChatTranscriptAgent(agentRef.current) === 'codex' + ? buildCodexAskAnswerKeys(prompt, selections) + : buildAskAnswerKeys(prompt, selections) + for (let index = 0; index < groups.length; index += 1) { + if (generationRef.current !== generation) { + return false + } + const group = groups[index]! + const body = 'raw' in group ? group.raw : sanitizeAskFreeText(group.text) + if (!(await sendTerminal(body, false))) { + return fail() + } + if (index < groups.length - 1) { + if (!(await wait(MOBILE_NATIVE_CHAT_QUESTION_STEP_MS))) { + return false + } + // Pacing is deliberate, not transport latency — don't charge it to the budget. + deadline += MOBILE_NATIVE_CHAT_QUESTION_STEP_MS + } + } + // Taken over on the last key: same as above, the successor owns the surface. + return groups.length > 0 && writeTurnsRef.current.get(handle) === turn + } finally { + // Any accepted key changed the live selector, so a queued replacement + // cannot safely apply its from-scratch key plan to that new position. + finishTurn(predecessorSafe && !sawUnknownOutcome && !sawAcceptedGroup) + if (writeTurnsRef.current.get(handle) === turn) { + writeTurnsRef.current.delete(handle) + } + // Last chain out releases; a superseded chain unwinding late must not + // free the lock out from under the successor sharing its hold. + const remaining = (holds.get(handle) ?? 1) - 1 + if (remaining <= 0) { + holds.delete(handle) + releaseMobileNativeChatTerminalWrite(handle) + } else { + holds.set(handle, remaining) } } - return groups.length > 0 }, [ agentRef, diff --git a/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts b/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts index be2f9a373..13cb5d96c 100644 --- a/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts +++ b/mobile/src/session/use-mobile-native-chat-image-attachments.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { RpcClient } from '../transport/rpc-client' import type { RpcResponse, RpcSuccess } from '../transport/types' import { resetMobileNativeChatStaleInputForTests } from './mobile-native-chat-stale-input' +import { resetMobileNativeChatTerminalWritesForTests } from './mobile-native-chat-terminal-write-lock' import { useMobileNativeChatImageAttachments } from './use-mobile-native-chat-image-attachments' // Fully stub the picker so the real expo/react-native chain never loads under @@ -88,9 +89,10 @@ describe('useMobileNativeChatImageAttachments', () => { beforeEach(() => { globalThis.IS_REACT_ACT_ENVIRONMENT = true pick.mockReset() - // Stale markers live at module scope now (they outlive the screen), so they - // also outlive a test. + // Stale markers and write locks live at module scope (they outlive the + // screen), so they also outlive a test. resetMobileNativeChatStaleInputForTests() + resetMobileNativeChatTerminalWritesForTests() }) afterEach(() => { act(() => renderer?.unmount()) diff --git a/mobile/src/session/use-mobile-native-chat-image-attachments.ts b/mobile/src/session/use-mobile-native-chat-image-attachments.ts index a692b11ac..38f9e462e 100644 --- a/mobile/src/session/use-mobile-native-chat-image-attachments.ts +++ b/mobile/src/session/use-mobile-native-chat-image-attachments.ts @@ -26,6 +26,10 @@ import { isMobileNativeChatInputStale, markMobileNativeChatInputStale } from './mobile-native-chat-stale-input' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite +} from './mobile-native-chat-terminal-write-lock' type CurrentRef = { readonly current: T } type ShowToast = (message: string, durationMs?: number) => void @@ -127,8 +131,6 @@ export function useMobileNativeChatImageAttachments({ // checked 'connected' at entry, so only a ref can see a mid-upload disconnect. const connStateRef = useRef(connState) connStateRef.current = connState - // Serialize clear/paste/submit ownership per terminal while allowing other tabs to send. - const sendInFlightTerminalsRef = useRef(new Set()) const attachments = (scopeKey ? attachmentsByScope[scopeKey] : undefined) ?? NO_ATTACHMENTS @@ -219,15 +221,15 @@ export function useMobileNativeChatImageAttachments({ const sendNativeChat = useCallback( async (text: string): Promise => { + // Serialize clear/paste/submit ownership per terminal while allowing other + // tabs to send. Shared with the prompt-card writes (answer/permission), so + // a card tap can't interleave into a mid-flight paste sequence either. const operationTerminal = activeHandleRef.current - if (operationTerminal && sendInFlightTerminalsRef.current.has(operationTerminal)) { + if (operationTerminal && !acquireMobileNativeChatTerminalWrite(operationTerminal)) { onError?.() onSendError('Message not sent') return false } - if (operationTerminal) { - sendInFlightTerminalsRef.current.add(operationTerminal) - } // One budget for the whole user action. The paste loop, the settle, and the // text body that follows are a single send from the composer's point of view; // opening a budget per leg let `sending` run to twice the stated ceiling. @@ -347,7 +349,7 @@ export function useMobileNativeChatImageAttachments({ } } finally { if (operationTerminal) { - sendInFlightTerminalsRef.current.delete(operationTerminal) + releaseMobileNativeChatTerminalWrite(operationTerminal) } } }, diff --git a/mobile/src/session/use-mobile-native-chat-message-send.test.ts b/mobile/src/session/use-mobile-native-chat-message-send.test.ts index da36cdb92..7c7c8e581 100644 --- a/mobile/src/session/use-mobile-native-chat-message-send.test.ts +++ b/mobile/src/session/use-mobile-native-chat-message-send.test.ts @@ -19,6 +19,11 @@ vi.mock('./mobile-native-chat-stale-input', () => ({ })) import { useMobileNativeChatMessageSend } from './use-mobile-native-chat-message-send' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite, + resetMobileNativeChatTerminalWritesForTests +} from './mobile-native-chat-terminal-write-lock' import { buildAgentTuiClearInputForText } from '../../../src/shared/agent-tui-input-clear' type Send = ReturnType @@ -28,6 +33,7 @@ const DRAFT = 'Linked Linear issue: ABC-123\nhttps://linear.app/x/issue/ABC-123' describe('useMobileNativeChatMessageSend', () => { let renderer: ReactTestRenderer | null = null let api: Send | null = null + let onSendError = vi.fn() const mount = ( readSeededLaunchDraftSeed: () => { text: string; createdAt: number | null } | null @@ -44,7 +50,7 @@ describe('useMobileNativeChatMessageSend', () => { restoreRejectedDraft: () => {}, acceptSend: () => {}, holdUnconfirmedSend: () => {}, - onSendError: () => {} + onSendError }) return null } @@ -70,6 +76,8 @@ describe('useMobileNativeChatMessageSend', () => { sendWithOutcome.mockResolvedValue('accepted') clearInputWrite.mockReset() clearInputWrite.mockResolvedValue(true) + onSendError = vi.fn() + resetMobileNativeChatTerminalWritesForTests() }) afterEach(() => { act(() => { @@ -173,4 +181,27 @@ describe('useMobileNativeChatMessageSend', () => { }) expect(sentArgs().resolvedLaunchDraft).toBeUndefined() }) + + it('rejects a question answer while another composed write holds the terminal', async () => { + mount(() => null) + // An image paste sequence is mid-flight into the same PTY. + expect(acquireMobileNativeChatTerminalWrite('term')).toBe(true) + + let result: boolean | undefined + await act(async () => { + result = await api!.answerQuestion('1') + }) + expect(result).toBe(false) + expect(sendWithOutcome).not.toHaveBeenCalled() + expect(onSendError).toHaveBeenCalledWith('Answer not sent') + + releaseMobileNativeChatTerminalWrite('term') + await act(async () => { + result = await api!.answerQuestion('1') + }) + expect(result).toBe(true) + // The answer released its own hold on the way out. + expect(acquireMobileNativeChatTerminalWrite('term')).toBe(true) + releaseMobileNativeChatTerminalWrite('term') + }) }) diff --git a/mobile/src/session/use-mobile-native-chat-message-send.ts b/mobile/src/session/use-mobile-native-chat-message-send.ts index 4143af338..7847f83c8 100644 --- a/mobile/src/session/use-mobile-native-chat-message-send.ts +++ b/mobile/src/session/use-mobile-native-chat-message-send.ts @@ -7,6 +7,10 @@ import { type MobileNativeChatSendOutcome } from './mobile-native-chat-send' import { healMobileNativeChatStaleInput } from './mobile-native-chat-stale-input' +import { + acquireMobileNativeChatTerminalWrite, + releaseMobileNativeChatTerminalWrite +} from './mobile-native-chat-terminal-write-lock' import type { MobileNativeChatSendOrigin } from './use-mobile-native-chat-drafts' import type { MobileNativeChatLaunchDraftSeed } from './use-mobile-native-chat-launch-draft-seed' import { buildAgentTuiClearInputForText } from '../../../src/shared/agent-tui-input-clear' @@ -210,11 +214,26 @@ export function useMobileNativeChatMessageSend(args: { [sendWithOutcome] ) - // A question answer is not composer text, so it never syncs the draft. + // A question answer is not composer text, so it never syncs the draft. It + // reaches this send directly (not through the image hook's locked path), so + // it takes the per-terminal write lock itself: an answer landing mid-flight + // in an image paste sequence would interleave bytes into the PTY. const answerQuestion = useCallback( - async (text: string): Promise => - (await sendMessage(text, undefined, false)) !== 'rejected', - [sendMessage] + async (text: string): Promise => { + const terminal = handleRef.current + if (terminal && !acquireMobileNativeChatTerminalWrite(terminal)) { + onSendError('Answer not sent') + return false + } + try { + return (await sendMessage(text, undefined, false)) !== 'rejected' + } finally { + if (terminal) { + releaseMobileNativeChatTerminalWrite(terminal) + } + } + }, + [handleRef, onSendError, sendMessage] ) return { send, sendWithOutcome, answerQuestion }