Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions mobile/src/session/mobile-structured-send-delivery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,9 @@ describe('mobileStructuredSendDelivery', () => {
})
})

it('never releases an ambiguous id on a later RPC refusal or failure', () => {
it('spends an ambiguous id the host has expired, and says to check the chat', () => {
// The host refuses an expired id on every replay; keeping it would refuse this text forever.
// The earlier attempt may still be in the chat, so the words never say it was not sent.
expect(
mobileStructuredSendDelivery(
{
Expand All @@ -179,7 +181,25 @@ describe('mobileStructuredSendDelivery', () => {
},
true
)
).toEqual({ outcome: 'rejected', operationIdSpent: false, error: 'Operation expired' })
).toEqual({
outcome: 'rejected',
operationIdSpent: true,
error:
"Orca couldn't confirm your message reached the agent. Check the chat, then send it again if needed."
})
})

it('never releases an ambiguous id on any other later RPC refusal or failure', () => {
expect(
mobileStructuredSendDelivery(
{
status: 'refused',
code: 'agent_session_operation_conflict',
message: 'Operation conflict'
},
true
)
).toEqual({ outcome: 'rejected', operationIdSpent: false, error: 'Operation conflict' })
expect(
mobileStructuredSendDelivery(
{ status: 'failed', message: 'Your message was not sent. Send it again.' },
Expand Down
20 changes: 15 additions & 5 deletions mobile/src/session/mobile-structured-send-delivery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
// message is a new message and must carry a new id.
// rejected — a terminal refusal or rejected submission spends a fresh id. A
// pending-admission refusal, or any refusal after earlier transport doubt,
// keeps it because neither proves a retained delivery did not happen. The
// one exception is a host that refuses the replay's request shape itself
// (an older host's strict schema turning `delivery` away): that host can
// never accept the replay, so keeping the id would only refuse every later
// send of the same text.
// keeps it because neither proves a retained delivery did not happen. Two
// exceptions spend it anyway, because the host can never accept the replay
// and keeping the id would only refuse every later send of the same text:
// a host that refuses the replay's request shape itself (an older host's
// strict schema turning `delivery` away), and an id the host has expired.
// unknown — the one answer that KEEPS its id, whether it came from the host or
// from an ack-loss on the way back. The message may be with the provider, so
// the retry has to stay a replay. Rotating here is what sent one message to a
Expand All @@ -28,6 +28,7 @@
import type { AgentJournalSubmission } from '../../../src/shared/agent-session-journal-types'
import type { AgentSessionSendResult } from '../../../src/shared/agent-session-wire'
import { agentSessionRefusalOperationState } from '../../../src/shared/agent-session-refusal-retry'
import { agentSessionWriteNoticeEnglish } from '../../../src/shared/agent-session-refusal-notice'
import { structuredAgentSessionRejectionNotice } from '../../../src/shared/structured-agent-session-send-disposition'
import type { MobileNativeChatSendOutcome } from './mobile-native-chat-send'
import type { StructuredAgentSessionMutationCallResult } from './mobile-structured-agent-session-rpc'
Expand All @@ -52,6 +53,15 @@ export function mobileStructuredSendDelivery(
if (refusalState === 'unknown') {
return { outcome: 'unknown', operationIdSpent: false, error: null }
}
if (retained && result.code === 'agent_session_operation_expired') {
// The host refuses this id for good once its day is up, so keeping it would refuse this text
// forever. The earlier attempt may already be in the chat, so the words say to check first.
return {
outcome: 'rejected',
operationIdSpent: true,
error: agentSessionWriteNoticeEnglish(['sendOutcomeLost'])
}
}
return {
outcome: 'rejected',
operationIdSpent: refusalState === 'settled-rejected' && !retained,
Expand Down
8 changes: 4 additions & 4 deletions mobile/src/session/use-mobile-native-chat-drafts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ describe('useMobileNativeChatDrafts', () => {
expect(state?.composerText).toBe('ping')
})

it('does not clobber newer edits when restoring a rejected send', async () => {
it('appends a rejected send after edits typed while it was in flight', async () => {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
Expand All @@ -157,10 +157,10 @@ describe('useMobileNativeChatDrafts', () => {
state?.restoreRejectedDraft(origin, 'ping')
}
})
expect(state?.composerText).toBe('newer edit')
expect(state?.composerText).toBe('newer edit\n\nping')
})

it('preserves an intentional clear after a newer edit while a rejection is pending', async () => {
it('returns a rejected send even after a newer edit was cleared', async () => {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
Expand All @@ -177,7 +177,7 @@ describe('useMobileNativeChatDrafts', () => {
}
})

expect(state?.composerText).toBe('')
expect(state?.composerText).toBe('ping')
})

it('restores a rejected send onto its originating tab only', async () => {
Expand Down
16 changes: 8 additions & 8 deletions mobile/src/session/use-mobile-native-chat-drafts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
type SetStateAction
} from 'react'
import type { NativeChatMessage } from '../../../src/shared/native-chat-types'
import { appendReturnedDraftText } from '../../../src/shared/returned-draft-text'
import {
countUserTextOccurrences,
findLandedImagePreviewEchoes,
Expand Down Expand Up @@ -77,7 +78,7 @@ export function useMobileNativeChatDrafts(args: {
readSeededLaunchDraftSeed: () => MobileNativeChatLaunchDraftSeed | null
/** Clear the composer at send time, before the RPC settles. */
clearDraftForSend: (origin: MobileNativeChatSendOrigin, text: string) => void
/** Put the text back after a definite rejection, unless newer edits exist. */
/** Put the text back after a definite rejection, after whatever the composer holds now. */
restoreRejectedDraft: (origin: MobileNativeChatSendOrigin, text: string) => void
acceptSend: (origin: MobileNativeChatSendOrigin, text: string, images?: string[]) => void
holdUnconfirmedSend: (
Expand Down Expand Up @@ -200,13 +201,12 @@ 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) =>
draftEditGenerationsRef.current.isCurrent(origin.draftKey, origin.draftEditGeneration) &&
(previous[origin.draftKey] ?? '') === ''
? { ...previous, [origin.draftKey]: text }
: previous
)
// Appended, so typing done while the send was in flight stays and the returned text isn't dropped.
setDrafts((previous) => {
const current = previous[origin.draftKey] ?? ''
const next = appendReturnedDraftText(current, text)
return next === current ? previous : { ...previous, [origin.draftKey]: next }
})
}, [])

const acceptSend = useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ describe('mobile structured send retries', () => {
expect(calls().every(([, params]) => !('retryUnknown' in (params as object)))).toBe(true)
})

it('keeps an ambiguous id after the host replay window expires', async () => {
it('sends under a new id once the host has expired an ambiguous one', async () => {
let attempts = 0
sendRequest.mockImplementation(async (method) => {
if (method !== 'agentSession.send') {
Expand All @@ -400,6 +400,9 @@ describe('mobile structured send retries', () => {
if (attempts === 1) {
throw markRpcDeliveryUnknown(new Error('Connection closed'))
}
if (attempts === 3) {
return sendResult('accepted')
}
return ok({
ok: false,
refusal: {
Expand All @@ -413,11 +416,16 @@ describe('mobile structured send retries', () => {
await act(async () => {
expect(await hook!.sendWithOutcome('old ambiguity')).toBe('unknown')
expect(await hook!.sendWithOutcome('old ambiguity')).toBe('rejected')
expect(await hook!.sendWithOutcome('old ambiguity')).toBe('rejected')
expect(await hook!.sendWithOutcome('old ambiguity')).toBe('accepted')
})

expect(calls()).toHaveLength(3)
expect(new Set(sentIds()).size).toBe(1)
const [first, replay, fresh] = sentIds()
expect(replay).toBe(first)
expect(fresh).not.toBe(first)
expect(onSendError).toHaveBeenCalledWith(
"Orca couldn't confirm your message reached the agent. Check the chat, then send it again if needed."
)
})

it('does not retain an id when the action budget expires before dispatch', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { JSONContent } from '@tiptap/react'
// draft would be lost on every TUI/GUI round-trip. Mirrors the attachment cache
// so both halves of an unsent message survive toggles and reconnects.

import { appendReturnedDraftText } from '../../../../shared/returned-draft-text'
import { setBoundedScopeCacheEntry } from './native-chat-composer-scope-cache'

const draftCache = new Map<string, { text: string; document?: JSONContent }>()
Expand All @@ -28,10 +29,6 @@ export function writeNativeChatDraftCache(scopeKey: string, draft: string): void
})
}

export function appendNativeChatDraftText(draft: string, text: string): string {
return draft === '' ? text : `${draft.trimEnd()}\n\n${text}`
}

// Only a write from outside the composer notifies; its own writes already hold the text.
const appendListeners = new Map<string, Set<(text: string) => void>>()

Expand All @@ -42,7 +39,7 @@ export function appendNativeChatDraftCache(scopeKey: string, text: string): void
}
writeNativeChatDraftCache(
scopeKey,
appendNativeChatDraftText(readNativeChatDraftCache(scopeKey), text)
appendReturnedDraftText(readNativeChatDraftCache(scopeKey), text)
)
appendListeners.get(scopeKey)?.forEach((listener) => listener(text))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,13 @@ describe('a message the host withdrew at a Stop', () => {
unsubscribe()

expect(storedAtRestore).toEqual([id])
// The drop never reached storage: the next mount gives the text back again rather than losing it.
// The drop never reached storage: after a crash, which loses the in-memory draft, the next mount
// gives the text back again rather than losing it.
cleanup()
writeNativeChatDraftCache(PANE, '')
writeOutbox(SESSION, beforeDrop)
renderOutbox().rerender({ submissions: [withdrawn(id)] })
expect(readNativeChatDraftCache(PANE)).toBe('hello\n\nhello')
expect(readNativeChatDraftCache(PANE)).toBe('hello')
})

it('keeps a message refused for any other reason on its Retry, and gives nothing back', async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useCallback, useEffect, useRef, useState } from 'react'
import { appendReturnedDraftText } from '../../../../shared/returned-draft-text'
import {
appendNativeChatDraftText,
readNativeChatDraftCache,
subscribeToNativeChatDraftAppend,
writeNativeChatDraftCache
Expand Down Expand Up @@ -45,7 +45,7 @@ export function useNativeChatDraft(
pendingAppendRef.current = {
scopeKey,
text:
pending?.scopeKey === scopeKey ? appendNativeChatDraftText(pending.text, text) : text
pending?.scopeKey === scopeKey ? appendReturnedDraftText(pending.text, text) : text
}
}),
[isComposing, scopeKey]
Expand All @@ -61,7 +61,7 @@ export function useNativeChatDraft(
writeNativeChatDraftCache(
scopeKey,
pending?.scopeKey === scopeKey
? appendNativeChatDraftText(resolved, pending.text)
? appendReturnedDraftText(resolved, pending.text)
: resolved
)
return resolved
Expand All @@ -74,7 +74,7 @@ export function useNativeChatDraft(
const pending = pendingAppendRef.current
pendingAppendRef.current = null
if (pending?.scopeKey === scopeKey) {
setDraft((previous) => appendNativeChatDraftText(previous, pending.text))
setDraft((previous) => appendReturnedDraftText(previous, pending.text))
}
}, [scopeKey, setDraft])

Expand Down
23 changes: 23 additions & 0 deletions src/shared/returned-draft-text.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { describe, expect, it } from 'vitest'
import { appendReturnedDraftText } from './returned-draft-text'

describe('appendReturnedDraftText', () => {
it('fills an empty composer with the returned text as it was typed', () => {
expect(appendReturnedDraftText('', 'ping ')).toBe('ping ')
expect(appendReturnedDraftText(' \n', 'ping')).toBe('ping')
})

it('puts the returned text after a draft, a blank line apart', () => {
expect(appendReturnedDraftText('newer edit\n', 'ping')).toBe('newer edit\n\nping')
})

it('adds nothing when the draft already ends with that text', () => {
expect(appendReturnedDraftText('ping', 'ping')).toBe('ping')
expect(appendReturnedDraftText('first\n\nping\n', 'ping ')).toBe('first\n\nping\n')
expect(appendReturnedDraftText('ping', '')).toBe('ping')
})

it('still appends text that only ends a line of the draft', () => {
expect(appendReturnedDraftText('say ping', 'ping')).toBe('say ping\n\nping')
})
})
19 changes: 19 additions & 0 deletions src/shared/returned-draft-text.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Puts text a chat handed back after what the composer already holds, a blank line apart. It never
* replaces typed text (a whitespace-only draft counts as empty), and adds nothing when the draft
* already ends with that text, so handing the same text back twice leaves one copy.
*/
export function appendReturnedDraftText(draft: string, text: string): string {
const returned = text.trimEnd()
if (returned.trim() === '') {
return draft
}
const kept = draft.trimEnd()
if (kept === '') {
return text
}
if (kept === returned || kept.endsWith(`\n\n${returned}`)) {
return draft
}
return `${kept}\n\n${text}`
}
Loading