From 6289065cda5a3a1e17ea2225c1ec711ba2ed572e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:06:31 -0700 Subject: [PATCH 01/23] fix(native-chat): a message the host recorded stays in the chat when it is not delivered A send the host recorded and then settled as not delivered (its agent failed to start, Orca restarted before handing it over) was hidden by the transcript projection unless the sender's outbox entry still carried it. Another window, the phone, or the sender after its entry was dropped saw the message vanish. The projection now shows every recorded, non-withdrawn rejection from the journal as a not-sent row at its journal position, and the delivery notices word it from the host's fact (no Retry where this client holds nothing to send). The phone row says "Message not sent". Not-sent rows take no rail tick, so the host's outline is unchanged. One predicate now reads a recovered unknown, including an older host's restart row without the marker. --- .../session/MobileNativeChatMessage.test.ts | 10 ++ .../src/session/MobileNativeChatMessage.tsx | 4 + .../mobile-native-chat-message-styles.ts | 5 + .../native-chat-message-rail-items.ts | 3 +- ...red-agent-session-delivery-notices.test.ts | 101 ++++++++++++++++++ ...ructured-agent-session-delivery-notices.ts | 35 +++++- ...d-agent-session-message-projection.test.ts | 70 +++++++++--- .../agent-session-conversation-outline.ts | 2 + ...ctured-agent-session-message-projection.ts | 29 +++-- ...red-agent-session-send-disposition.test.ts | 27 +++++ ...ructured-agent-session-send-disposition.ts | 3 +- ...-agent-session-unanswered-dispatch.test.ts | 48 +++++++++ ...tured-agent-session-unanswered-dispatch.ts | 17 ++- 13 files changed, 327 insertions(+), 27 deletions(-) create mode 100644 src/shared/structured-agent-session-unanswered-dispatch.test.ts diff --git a/mobile/src/session/MobileNativeChatMessage.test.ts b/mobile/src/session/MobileNativeChatMessage.test.ts index 67e90132866..e7e9ae80a6f 100644 --- a/mobile/src/session/MobileNativeChatMessage.test.ts +++ b/mobile/src/session/MobileNativeChatMessage.test.ts @@ -107,6 +107,16 @@ describe('MobileNativeChatMessage', () => { expect(texts.some((text) => text.includes('/tmp/host.png'))).toBe(true) }) + it('says under a message the host recorded but never delivered that it was not sent', () => { + const tree = render({ ...userMessage([{ type: 'text', text: 'hello' }]), unsent: true }) + expect(textIn(tree.root)).toEqual(['hello', 'Message not sent']) + }) + + it('says nothing more under a delivered message', () => { + const tree = render(userMessage([{ type: 'text', text: 'hello' }])) + expect(textIn(tree.root)).toEqual(['hello']) + }) + it('makes user message text selectable', () => { const tree = render(userMessage([{ type: 'text', text: 'Prompt I typed' }])) const text = tree.root diff --git a/mobile/src/session/MobileNativeChatMessage.tsx b/mobile/src/session/MobileNativeChatMessage.tsx index 9c894e3cd7f..01b9dd465c5 100644 --- a/mobile/src/session/MobileNativeChatMessage.tsx +++ b/mobile/src/session/MobileNativeChatMessage.tsx @@ -158,6 +158,10 @@ function MobileNativeChatMessageImpl({ /> ) : null} + {/* The host recorded it and never delivered it; the desktop row words the same fact. */} + {isUser && message.unsent === true ? ( + Message not sent + ) : null} {turnStatusAbove ? null : statusRow} diff --git a/mobile/src/session/mobile-native-chat-message-styles.ts b/mobile/src/session/mobile-native-chat-message-styles.ts index 51ff9ab1f36..b3455c89830 100644 --- a/mobile/src/session/mobile-native-chat-message-styles.ts +++ b/mobile/src/session/mobile-native-chat-message-styles.ts @@ -32,6 +32,11 @@ export const styles = StyleSheet.create({ reasoning: { opacity: 0.7 }, + unsentLabel: { + marginTop: spacing.xs, + color: colors.statusRed, + fontSize: typography.metaSize + }, toolRun: { marginTop: spacing.xs }, diff --git a/src/renderer/src/components/native-chat/native-chat-message-rail-items.ts b/src/renderer/src/components/native-chat/native-chat-message-rail-items.ts index 1ad2ba71a39..1c0e371c9cb 100644 --- a/src/renderer/src/components/native-chat/native-chat-message-rail-items.ts +++ b/src/renderer/src/components/native-chat/native-chat-message-rail-items.ts @@ -39,7 +39,8 @@ export function buildNativeChatRailItems( ): readonly NativeChatRailItem[] { const items: NativeChatRailItem[] = [] for (const [slotIndex, slot] of slots.entries()) { - if (slot.kind !== 'message' || slot.message.role !== 'user') { + // A message shown as not sent is no turn of the conversation, as the host's outline agrees. + if (slot.kind !== 'message' || slot.message.role !== 'user' || slot.message.unsent === true) { continue } const preview = nativeChatUserMessagePreview(slot.message.blocks) diff --git a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts index 5dde72621ad..b2464e13e37 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts @@ -547,3 +547,104 @@ describe('the notice on each message that did not go through', () => { }) }) }) + +describe('a recorded rejection no outbox entry here carries', () => { + const recorded = (patch: Partial = {}): AgentJournalSubmission => ({ + clientMessageId: 'elsewhere', + fence: 1, + payloadFingerprint: 'fingerprint', + dispatchState: 'rejected', + providerItemId: null, + reason: 'not_delivered', + submittedAt: 1, + resolvedAt: 1, + ...patch + }) + + it('says it was not sent on its own row, with no Retry, in the words the sender would read', () => { + const notices = structuredAgentSessionDeliveryNotices( + [], + 'Claude', + () => {}, + [recorded()], + [], + NOT_FAILED_HERE + ) + const senderNotices = structuredAgentSessionDeliveryNotices( + [ + entry('elsewhere', { + state: 'rejected', + lastFailure: structuredAgentSessionRejectedFailure(recorded()) + }) + ], + 'Claude', + () => {}, + [recorded()], + [], + new Set(['elsewhere']) + ) + const id = agentJournalSubmissionKey('elsewhere') + expect(notices.get(id)?.onRetry).toBeUndefined() + expect(senderNotices.get(id)?.onRetry).toBeDefined() + expect(notices.get(id)?.text).toBe('Your message was not sent.') + expect(senderNotices.get(id)?.text).toBe(notices.get(id)?.text) + }) + + it("keeps the sender's Retry while its outbox entry exists", () => { + const retry = vi.fn() + const notices = structuredAgentSessionDeliveryNotices( + [ + entry('mine', { + state: 'rejected', + lastFailure: structuredAgentSessionRejectedFailure(recorded({ clientMessageId: 'mine' })) + }) + ], + 'Claude', + retry, + [recorded({ clientMessageId: 'mine' })], + [], + NOT_FAILED_HERE + ) + notices.get(agentJournalSubmissionKey('mine'))?.onRetry?.() + expect(retry).toHaveBeenCalledWith('mine') + expect(notices.size).toBe(1) + }) + + it('says nothing for a send a Stop withdrew', () => { + const notices = structuredAgentSessionDeliveryNotices( + [], + 'Claude', + () => {}, + [recorded({ reason: 'provider_cancelled_before_start' })], + [], + NOT_FAILED_HERE + ) + expect(notices.size).toBe(0) + }) + + it('says only that it was not sent when the start-failure row already says why', () => { + const startFailed: AgentSessionFailureFact = { kind: 'startFailed' } + const row: AgentJournalRenderItem = { + itemId: agentJournalItemKey(structuredAgentSessionStartFailureRowIdentity('gen')), + revision: 1, + sequence: 1, + observedAt: 1, + body: { + kind: 'status', + tone: 'error', + ...agentSessionFailureWords(startFailed, { agentName: 'Claude', surface: 'row' }) + } + } + const notices = structuredAgentSessionDeliveryNotices( + [], + 'Claude', + () => {}, + [recorded({ reason: 'Written by the host.', rejection: startFailed })], + structuredAgentSessionStartFailureFacts([row]), + NOT_FAILED_HERE + ) + expect(notices.get(agentJournalSubmissionKey('elsewhere'))?.text).toBe( + 'Your message was not sent.' + ) + }) +}) diff --git a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts index 6ad655e8f80..912372f1a70 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts @@ -10,7 +10,8 @@ // A message the host recorded and then rejected is worded from the journal's own fact, found by id; // the message keeps only a smaller copy, read when its submission is not loaded. A rejection that // is a failed start's, the fact its loaded row states, says only that it was not sent: the row -// already says why. +// already says why. One no outbox entry here carries (another client's send, or one whose entry is +// gone) is worded from the journal alone, with no Retry: this client holds nothing to send. import { readAgentSessionFailureFact, @@ -26,8 +27,10 @@ import { agentSessionWriteNotDoneParts } from '../../../../shared/agent-session- import { isStructuredAgentSessionStartFailureRow } from '../../../../shared/structured-agent-session-start-failure-row-key' import { structuredAgentSessionEntryIdExpired, + structuredAgentSessionRejectedFailure, type StructuredAgentSessionOutboxEntry } from '../../../../shared/structured-agent-session-outbox' +import { dispatchWasWithdrawn } from '../../../../shared/structured-agent-session-dispatch-rejection' import { admitStructuredAgentSessionOutboxEntry, structuredAgentSessionEntryHeldForRetry @@ -129,6 +132,24 @@ function deliveryNoticeText( ) } +/** What a recorded rejection's own row says when no outbox entry here carries it. */ +function recordedRejectionNoticeText( + submission: AgentJournalSubmission, + context: AgentSessionFailureWordsContext, + startFailures: readonly AgentSessionFailureFact[] +): string { + if (agentSessionFailureStatedByStartRow(submission.rejection, startFailures)) { + return agentSessionWriteNoticeText(agentSessionWriteNotDoneParts('send')) + } + return agentSessionWriteNoticeText( + structuredAgentSessionAttemptFailureParts( + structuredAgentSessionRejectedFailure(submission), + context, + readWholeAgentSessionFailureFact(submission.rejection) + ) + ) +} + /** Keyed by the message id the transcript renders each entry under; `agentName` is the chat's * agent, for the words. */ export function structuredAgentSessionDeliveryNotices( @@ -172,5 +193,17 @@ export function structuredAgentSessionDeliveryNotices( ) } } + for (const submission of rejected.values()) { + const id = agentJournalSubmissionKey(submission.clientMessageId) + if (!notices.has(id) && !dispatchWasWithdrawn(submission)) { + notices.set(id, { + text: recordedRejectionNoticeText( + submission, + { agentName, retryControl: false }, + startFailures + ) + }) + } + } return notices } diff --git a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts index 3a3562458cb..2b9ba38f897 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts @@ -31,28 +31,72 @@ function item(index: number): AgentJournalRenderItem { } describe('structured agent session message projection', () => { - it('does not render a rejected host submission as a sent user message', () => { + it('shows a recorded send the host did not deliver as not sent, never as sent', () => { const rejected = { ...submission(0), dispatchState: 'rejected' as const, providerItemId: null } const refusedItem = { ...item(0), itemId: agentJournalSubmissionKey(rejected.clientMessageId) } const acceptedItem = item(1) expect( projectStructuredAgentSessionMessages([refusedItem, acceptedItem], [], [rejected]) - ).toMatchObject([{ id: acceptedItem.itemId, role: 'user' }]) + ).toMatchObject([ + { id: refusedItem.itemId, role: 'user', unsent: true }, + { id: acceptedItem.itemId, role: 'user' } + ]) }) - it('keeps a refused local draft available through its outbox', () => { + it('keeps a recorded send in the chat after its outbox entry is gone and the host settles it undelivered', () => { + // Orca restarted mid-send: the entry left this client, then the next agent start proved the + // provider never took the message. + const notDelivered = { + ...submission(0), + dispatchState: 'rejected' as const, + providerItemId: null, + reason: 'not_delivered' + } + const recordedItem = { + ...item(0), + itemId: agentJournalSubmissionKey(notDelivered.clientMessageId) + } + const messages = projectStructuredAgentSessionMessages([recordedItem], [], [notDelivered]) + expect(messages).toMatchObject([ + { + id: recordedItem.itemId, + unsent: true, + blocks: [{ type: 'text', text: 'send 0' }], + journalPosition: { sequence: 0 } + } + ]) + }) + + it('leaves out a send a Stop withdrew: its text went back to its sender', () => { + const withdrawn = { + ...submission(0), + dispatchState: 'rejected' as const, + providerItemId: null, + reason: 'provider_cancelled_before_start' + } + const withdrawnItem = { + ...item(0), + itemId: agentJournalSubmissionKey(withdrawn.clientMessageId) + } + expect(projectStructuredAgentSessionMessages([withdrawnItem], [], [withdrawn])).toEqual([]) + }) + + it("shows the sender's recorded rejection once, from the journal, while its outbox entry holds the Retry", () => { const rejected = { ...submission(0), dispatchState: 'rejected' as const, providerItemId: null } const refusedItem = { ...item(0), itemId: agentJournalSubmissionKey(rejected.clientMessageId) } - const draft = createStructuredAgentSessionOutboxEntry({ - clientMessageId: rejected.clientMessageId, - sessionId: 'session-1', - text: 'An unsent draft', - attachments: [], - queuedAt: 1 - }) - expect(projectStructuredAgentSessionMessages([refusedItem], [draft], [rejected])).toMatchObject( - [{ id: refusedItem.itemId, blocks: [{ text: 'An unsent draft' }] }] - ) + const draft = { + ...createStructuredAgentSessionOutboxEntry({ + clientMessageId: rejected.clientMessageId, + sessionId: 'session-1', + text: 'send 0', + attachments: [], + queuedAt: 1 + }), + state: 'rejected' as const + } + expect(projectStructuredAgentSessionMessages([refusedItem], [draft], [rejected])).toEqual([ + expect.objectContaining({ id: refusedItem.itemId, unsent: true }) + ]) }) it.each([5, 10])('renders %i rapid accepted desktop sends exactly once', (sendCount) => { diff --git a/src/shared/agent-session-conversation-outline.ts b/src/shared/agent-session-conversation-outline.ts index 2b6bb846adb..c8aeecff9e9 100644 --- a/src/shared/agent-session-conversation-outline.ts +++ b/src/shared/agent-session-conversation-outline.ts @@ -96,6 +96,8 @@ export function projectAgentSessionConversationOutline( if ( sequence === undefined || message.role !== 'user' || + // Shown as not sent: never part of the conversation the agent saw, so no rail tick. + message.unsent === true || !nativeChatRowRendersContent(message.blocks) ) { continue diff --git a/src/shared/structured-agent-session-message-projection.ts b/src/shared/structured-agent-session-message-projection.ts index 5515b6478a2..5223b4c8485 100644 --- a/src/shared/structured-agent-session-message-projection.ts +++ b/src/shared/structured-agent-session-message-projection.ts @@ -7,6 +7,7 @@ import type { NativeChatMessage } from './native-chat-types' import type { StructuredAgentSessionOutboxEntry } from './structured-agent-session-outbox' import { structuredAgentSessionEntryHeldForRetry } from './structured-agent-session-outbox-admission' import { reconcileStructuredAgentSessionOutboxWithQueue } from './structured-agent-session-draft-hand-off' +import { dispatchWasWithdrawn } from './structured-agent-session-dispatch-rejection' import { projectStructuredItemsToNativeChat } from './structured-agent-session-projection' export function projectStructuredAgentSessionMessages( @@ -16,16 +17,26 @@ export function projectStructuredAgentSessionMessages( projectItems = projectStructuredItemsToNativeChat ): NativeChatMessage[] { const optimistic = reconcileStructuredAgentSessionOutboxWithQueue(outbox, submissions) - // Refused sends are ledger evidence, not conversation history; local drafts remain in the outbox. - const rejected = new Set( - submissions - .filter((submission) => submission.dispatchState === 'rejected') - .map((submission) => agentJournalSubmissionKey(submission.clientMessageId)) - ) + // A send the host recorded and then did not deliver stays in the conversation from the host's + // record, shown as not sent, for every viewer: dropping the sender's outbox entry can never make + // it vanish. Only a Stop's withdrawal leaves it, since its text went back to its sender. + const notSent = new Set() + const withdrawn = new Set() + for (const submission of submissions) { + if (submission.dispatchState !== 'rejected') { + continue + } + const key = agentJournalSubmissionKey(submission.clientMessageId) + if (dispatchWasWithdrawn(submission)) { + withdrawn.add(key) + } else { + notSent.add(key) + } + } const visibleItems: AgentJournalRenderItem[] = [] const refused = new Map() for (const item of items) { - if (rejected.has(item.itemId)) { + if (withdrawn.has(item.itemId)) { refused.set(item.itemId, item) } else { visibleItems.push(item) @@ -42,7 +53,9 @@ export function projectStructuredAgentSessionMessages( const delivered: NativeChatMessage[] = [] const held: NativeChatMessage[] = [] for (const message of projectItems(visibleItems)) { - if (queued.has(message.id)) { + if (notSent.has(message.id)) { + delivered.push({ ...message, unsent: true }) + } else if (queued.has(message.id)) { held.push({ ...message, queued: true }) } else { delivered.push(message) diff --git a/src/shared/structured-agent-session-send-disposition.test.ts b/src/shared/structured-agent-session-send-disposition.test.ts index 98ea2ff8b67..5514d422015 100644 --- a/src/shared/structured-agent-session-send-disposition.test.ts +++ b/src/shared/structured-agent-session-send-disposition.test.ts @@ -411,6 +411,33 @@ describe('ambiguous operation refusals', () => { expect(disposition.entries).toMatchObject([{ clientMessageId: 'fresh-id', state: 'rejected' }]) }) + it("parks an older host's restart answer that carries the reason but not the recovered marker", () => { + const result = rejectedWith(null) + if (!result.ok || !('submission' in result.value)) { + throw new Error('expected a send result') + } + result.value.submission = { + ...result.value.submission, + dispatchState: 'unknown', + reason: 'host_restarted_before_acknowledgement' + } + + const disposition = disposeStructuredAgentSessionSendResult({ + entries: [entry], + entry, + result, + createOperationId: () => 'unused' + }) + + expect(disposition.entries).toMatchObject([ + { + clientMessageId: entry.clientMessageId, + state: 'unconfirmed', + retryAfterUnknownSubmittedAt: -1 + } + ]) + }) + it('parks a recovered missing submission without polling forever', () => { const result = rejectedWith(null) if (!result.ok || !('submission' in result.value)) { diff --git a/src/shared/structured-agent-session-send-disposition.ts b/src/shared/structured-agent-session-send-disposition.ts index f717e54f2d4..a8f68057fd5 100644 --- a/src/shared/structured-agent-session-send-disposition.ts +++ b/src/shared/structured-agent-session-send-disposition.ts @@ -22,6 +22,7 @@ import { import type { AgentSessionFailureFact } from './agent-session-failure' import type { AgentSessionFailureWordsContext } from './agent-session-failure-words' import { classifyDispatchRejection } from './structured-agent-session-dispatch-rejection' +import { isRecoveredStructuredAgentSessionSubmission } from './structured-agent-session-unanswered-dispatch' import { classifyStructuredAgentSessionSendFailure, requeueStructuredAgentSessionSendRefusal, @@ -277,7 +278,7 @@ export function disposeStructuredAgentSessionSendResult( error: null } } - if (submission.dispatchState === 'unknown' && submission.recovered) { + if (isRecoveredStructuredAgentSessionSubmission(submission)) { return { entries: input.entries.map((candidate) => candidate.clientMessageId === input.entry.clientMessageId diff --git a/src/shared/structured-agent-session-unanswered-dispatch.test.ts b/src/shared/structured-agent-session-unanswered-dispatch.test.ts new file mode 100644 index 00000000000..39a67f880e7 --- /dev/null +++ b/src/shared/structured-agent-session-unanswered-dispatch.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from 'vitest' +import type { AgentJournalSubmission } from './agent-session-journal-types' +import { + isRecoveredStructuredAgentSessionSubmission, + isUnansweredStructuredAgentSessionDispatch +} from './structured-agent-session-unanswered-dispatch' + +function submission(patch: Partial): AgentJournalSubmission { + return { + clientMessageId: 'client-1', + fence: 1, + payloadFingerprint: 'fingerprint', + dispatchState: 'unknown', + providerItemId: null, + reason: null, + submittedAt: 1, + resolvedAt: 1, + ...patch + } +} + +describe('a send whose outcome the host lost for good', () => { + it('is one the host marked recovered', () => { + const row = submission({ recovered: true, reason: 'provider_exited_before_acknowledgement' }) + expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(true) + expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(false) + }) + + it("is an older host's restart row, which carries the reason without the marker", () => { + const row = submission({ reason: 'host_restarted_before_acknowledgement' }) + expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(true) + expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(false) + }) + + it('is never a live unknown, which something still running may answer', () => { + const row = submission({ reason: 'provider_ack_ambiguous' }) + expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(false) + expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(true) + }) + + it('is never a settled send, whatever its reason says', () => { + expect( + isRecoveredStructuredAgentSessionSubmission( + submission({ dispatchState: 'rejected', reason: 'host_restarted_before_acknowledgement' }) + ) + ).toBe(false) + }) +}) diff --git a/src/shared/structured-agent-session-unanswered-dispatch.ts b/src/shared/structured-agent-session-unanswered-dispatch.ts index 412b3e98196..2113525564d 100644 --- a/src/shared/structured-agent-session-unanswered-dispatch.ts +++ b/src/shared/structured-agent-session-unanswered-dispatch.ts @@ -1,6 +1,19 @@ import type { AgentJournalSubmission } from './agent-session-journal-types' import { isQueuedAgentJournalSubmission } from './agent-session-queued-submission' +/** A send whose outcome the host lost for good when the process that sent it went away (a restart, + * a provider exit, an idle release): nothing still running can answer it. */ +export function isRecoveredStructuredAgentSessionSubmission( + submission: Pick +): boolean { + return ( + submission.dispatchState === 'unknown' && + (submission.recovered === true || + // Older hosts publish the recovery reason but omit the optional marker. + submission.reason === 'host_restarted_before_acknowledgement') + ) +} + /** One send the provider has neither opened a turn for nor refused; the rule is explained on * `hasUnansweredStructuredAgentSessionDispatch`, which asks it of every send. */ export function isUnansweredStructuredAgentSessionDispatch( @@ -15,9 +28,7 @@ export function isUnansweredStructuredAgentSessionDispatch( (currentFence == null || submission.fence >= currentFence) && (submission.dispatchState === 'pending' || (submission.dispatchState === 'unknown' && - submission.recovered !== true && - // Older hosts publish the recovery reason but omit the optional marker. - submission.reason !== 'host_restarted_before_acknowledgement')) + !isRecoveredStructuredAgentSessionSubmission(submission))) ) } From 1b0f2a7b5820c846f9250fe87850fb144a6f54e9 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:37:46 -0700 Subject: [PATCH 02/23] fix(native-chat): a not-delivered message is said once, on its row, on the phone too The phone's banner repeated what the recorded message's row now says. A recorded rejection's words move to one shared function: the desktop row translates them, the phone row shows them in English, and a start-failure row that already says why leaves only "not sent". The phone's banner keeps only a Stop's withdrawal, which has no row. --- .../session/MobileNativeChatMessage.test.ts | 16 +++- .../src/session/MobileNativeChatMessage.tsx | 11 ++- mobile/src/session/MobileNativeChatView.tsx | 8 +- .../mobile-native-chat-unsent-notices.test.ts | 74 ++++++++++++++++ .../mobile-native-chat-unsent-notices.ts | 37 ++++++++ .../mobile-structured-send-delivery.test.ts | 26 +++--- .../mobile-structured-send-delivery.ts | 7 +- ...red-agent-session-delivery-notices.test.ts | 6 +- ...ructured-agent-session-delivery-notices.ts | 86 +++---------------- .../structured-conversation-command-send.ts | 2 +- ...tured-agent-session-start-failure-facts.ts | 2 +- .../use-structured-agent-session.ts | 2 +- src/shared/native-chat-types.ts | 4 +- ...-agent-session-recorded-rejection-words.ts | 78 +++++++++++++++++ 14 files changed, 255 insertions(+), 104 deletions(-) create mode 100644 mobile/src/session/mobile-native-chat-unsent-notices.test.ts create mode 100644 mobile/src/session/mobile-native-chat-unsent-notices.ts create mode 100644 src/shared/structured-agent-session-recorded-rejection-words.ts diff --git a/mobile/src/session/MobileNativeChatMessage.test.ts b/mobile/src/session/MobileNativeChatMessage.test.ts index e7e9ae80a6f..d96d26c3100 100644 --- a/mobile/src/session/MobileNativeChatMessage.test.ts +++ b/mobile/src/session/MobileNativeChatMessage.test.ts @@ -70,6 +70,7 @@ describe('MobileNativeChatMessage', () => { workedSeconds: number | null } | null onToggleTurn?: () => void + unsentNotice?: string } = {} ): ReactTestRenderer { act(() => { @@ -107,9 +108,20 @@ describe('MobileNativeChatMessage', () => { expect(texts.some((text) => text.includes('/tmp/host.png'))).toBe(true) }) - it('says under a message the host recorded but never delivered that it was not sent', () => { + it('says under a message the host recorded but never delivered why it was not sent', () => { + const tree = render( + { ...userMessage([{ type: 'text', text: 'hello' }]), unsent: true }, + { unsentNotice: "Orca couldn't reach the agent. Your message was not sent." } + ) + expect(textIn(tree.root)).toEqual([ + 'hello', + "Orca couldn't reach the agent. Your message was not sent." + ]) + }) + + it('still says it was not sent when no words for it are loaded', () => { const tree = render({ ...userMessage([{ type: 'text', text: 'hello' }]), unsent: true }) - expect(textIn(tree.root)).toEqual(['hello', 'Message not sent']) + expect(textIn(tree.root)).toEqual(['hello', 'Your message was not sent.']) }) it('says nothing more under a delivered message', () => { diff --git a/mobile/src/session/MobileNativeChatMessage.tsx b/mobile/src/session/MobileNativeChatMessage.tsx index 01b9dd465c5..4e7ebd76abe 100644 --- a/mobile/src/session/MobileNativeChatMessage.tsx +++ b/mobile/src/session/MobileNativeChatMessage.tsx @@ -11,6 +11,7 @@ import { ToolRun } from './MobileNativeChatToolRun' import type { NativeChatTurnStatus } from './use-mobile-native-chat-turn-status' import { isRenderableImageUri } from './mobile-native-chat-image-preview' import { styles, TEXT_SIZE } from './mobile-native-chat-message-styles' +import { AGENT_SESSION_WRITE_NOTICE_COPY } from '../../../src/shared/agent-session-write-notice-copy' function Prose({ block, @@ -76,7 +77,8 @@ function MobileNativeChatMessageImpl({ turnKey, onToggleTurn, activeTurnIsWorking, - structuredActivityUi = false + structuredActivityUi = false, + unsentNotice }: { message: NativeChatMessage toolsExpanded?: boolean @@ -97,6 +99,8 @@ function MobileNativeChatMessageImpl({ activeTurnIsWorking?: boolean /** Structured lane only: live tool progress plus the turn-status disclosure. */ structuredActivityUi?: boolean + /** Why the host did not deliver this message, when it is shown as not sent. */ + unsentNotice?: string }): React.JSX.Element { const isUser = message.role === 'user' const isReasoning = message.role === 'reasoning' @@ -158,9 +162,10 @@ function MobileNativeChatMessageImpl({ /> ) : null} - {/* The host recorded it and never delivered it; the desktop row words the same fact. */} {isUser && message.unsent === true ? ( - Message not sent + + {unsentNotice ?? AGENT_SESSION_WRITE_NOTICE_COPY.notDoneSend} + ) : null} {turnStatusAbove ? null : statusRow} diff --git a/mobile/src/session/MobileNativeChatView.tsx b/mobile/src/session/MobileNativeChatView.tsx index 469abba5388..e8c45dad398 100644 --- a/mobile/src/session/MobileNativeChatView.tsx +++ b/mobile/src/session/MobileNativeChatView.tsx @@ -40,6 +40,7 @@ import type { MobileChatPermission } from './mobile-native-chat-permission' import type { MobileChatQuestion } from './mobile-native-chat-question' import type { MobileNativeChatSessionOptionPickersProps } from './MobileNativeChatSessionOptionPickers' import { MobileNativeChatMessage } from './MobileNativeChatMessage' +import { mobileNativeChatUnsentNotices } from './mobile-native-chat-unsent-notices' import type { MobileNativeChatStatus } from './use-mobile-native-chat-session' /** Why the composer input is locked: the transport is disconnected, or the @@ -280,6 +281,10 @@ export function MobileNativeChatView({ const hasPendingStructuredInteraction = structuredActivityUi && (ask != null || permission != null || question != null) + const unsentNotices = useMemo( + () => mobileNativeChatUnsentNotices(turnJournal, agent ?? null), + [turnJournal, agent] + ) const renderItem = useCallback( ({ item, index }: { item: NativeChatMessage; index: number }) => ( ), - [toolsExpanded, fontScale, onOpenFile, structuredActivityUi, turns] + [toolsExpanded, fontScale, onOpenFile, structuredActivityUi, turns, unsentNotices] ) const liveStatus = diff --git a/mobile/src/session/mobile-native-chat-unsent-notices.test.ts b/mobile/src/session/mobile-native-chat-unsent-notices.test.ts new file mode 100644 index 00000000000..5f4b503e06d --- /dev/null +++ b/mobile/src/session/mobile-native-chat-unsent-notices.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from 'vitest' +import type { AgentSessionFailureFact } from '../../../src/shared/agent-session-failure' +import { agentSessionFailureWords } from '../../../src/shared/agent-session-failure-words' +import { + agentJournalItemKey, + agentJournalSubmissionKey +} from '../../../src/shared/agent-session-journal-item-key' +import type { + AgentJournalRenderItem, + AgentJournalSubmission +} from '../../../src/shared/agent-session-journal-types' +import { structuredAgentSessionStartFailureRowIdentity } from '../../../src/shared/structured-agent-session-start-failure-row-key' +import { mobileNativeChatUnsentNotices } from './mobile-native-chat-unsent-notices' + +function rejected(patch: Partial = {}): AgentJournalSubmission { + return { + clientMessageId: 'sent-elsewhere', + fence: 1, + payloadFingerprint: 'fingerprint', + dispatchState: 'rejected', + providerItemId: null, + reason: 'provider_write_failed: broken pipe', + submittedAt: 1, + resolvedAt: 1, + ...patch + } +} + +describe('the line under a message the host recorded and did not deliver', () => { + it("says why, in the host's words, keyed by the message's row", () => { + const notices = mobileNativeChatUnsentNotices( + { items: [], submissions: [rejected()] }, + 'claude' + ) + expect(notices.get(agentJournalSubmissionKey('sent-elsewhere'))).toBe( + "Orca couldn't reach the agent. Your message was not sent." + ) + }) + + it('says only that it was not sent when the start-failure row already says why', () => { + const startFailed: AgentSessionFailureFact = { kind: 'startFailed' } + const startRow: AgentJournalRenderItem = { + itemId: agentJournalItemKey(structuredAgentSessionStartFailureRowIdentity('gen')), + revision: 1, + sequence: 1, + observedAt: 1, + body: { + kind: 'status', + tone: 'error', + ...agentSessionFailureWords(startFailed, { agentName: 'Claude', surface: 'row' }) + } + } + const notices = mobileNativeChatUnsentNotices( + { + items: [startRow], + submissions: [rejected({ reason: 'Written by the host.', rejection: startFailed })] + }, + 'claude' + ) + expect(notices.get(agentJournalSubmissionKey('sent-elsewhere'))).toBe( + 'Your message was not sent.' + ) + }) + + it('has nothing to say when no send was rejected', () => { + expect( + mobileNativeChatUnsentNotices( + { items: [], submissions: [rejected({ dispatchState: 'accepted' })] }, + 'claude' + ).size + ).toBe(0) + expect(mobileNativeChatUnsentNotices(null, 'claude').size).toBe(0) + }) +}) diff --git a/mobile/src/session/mobile-native-chat-unsent-notices.ts b/mobile/src/session/mobile-native-chat-unsent-notices.ts new file mode 100644 index 00000000000..d04cc894012 --- /dev/null +++ b/mobile/src/session/mobile-native-chat-unsent-notices.ts @@ -0,0 +1,37 @@ +// The line under each message the host recorded and then did not deliver: the desktop row's words, +// in English. Only the transcript says it, so the phone's own send never repeats it in its banner. + +import { formatAgentTypeLabel } from '../../../src/shared/agent-type-label' +import { agentJournalSubmissionKey } from '../../../src/shared/agent-session-journal-item-key' +import { agentSessionWriteNoticeEnglish } from '../../../src/shared/agent-session-refusal-notice' +import type { NativeChatTurnJournal } from '../../../src/shared/native-chat-turn-membership' +import { + structuredAgentSessionRecordedRejectionParts, + structuredAgentSessionStartFailureFacts +} from '../../../src/shared/structured-agent-session-recorded-rejection-words' + +const NO_NOTICES: ReadonlyMap = new Map() + +/** Keyed by the message id the transcript renders each recorded send under. */ +export function mobileNativeChatUnsentNotices( + journal: NativeChatTurnJournal | null, + agent: string | null +): ReadonlyMap { + const rejected = journal?.submissions.filter((row) => row.dispatchState === 'rejected') ?? [] + if (!journal || rejected.length === 0) { + return NO_NOTICES + } + const startFailures = structuredAgentSessionStartFailureFacts(journal.items) + const context = { + retryControl: false, + ...(agent ? { agentName: formatAgentTypeLabel(agent) } : {}) + } + return new Map( + rejected.map((submission) => [ + agentJournalSubmissionKey(submission.clientMessageId), + agentSessionWriteNoticeEnglish( + structuredAgentSessionRecordedRejectionParts(submission, context, startFailures) + ) + ]) + ) +} diff --git a/mobile/src/session/mobile-structured-send-delivery.test.ts b/mobile/src/session/mobile-structured-send-delivery.test.ts index 6305bf75df5..bad17106e16 100644 --- a/mobile/src/session/mobile-structured-send-delivery.test.ts +++ b/mobile/src/session/mobile-structured-send-delivery.test.ts @@ -112,26 +112,26 @@ describe('mobileStructuredSendDelivery', () => { } }) - it('spends the id of a rejection and withholds its internal reason', () => { + it('spends the id of a recorded rejection and leaves saying it to its row in the chat', () => { // Provably undelivered and terminal, so the id can only replay it: spending the - // id makes the retry a first delivery. The marker itself names nothing a person - // can act on, so it must not reach the screen. - expect( - mobileStructuredSendDelivery(accepted('rejected', 'provider_write_failed: broken pipe')) - ).toEqual({ - outcome: 'rejected', - operationIdSpent: true, - error: "Orca couldn't reach the agent. Your message was not sent. Send it again." - }) + // id makes the retry a first delivery. The host recorded the message, so the + // transcript shows it as not sent, with why; a banner would say it twice. + for (const reason of ['provider_write_failed: broken pipe', 'Claude does not support .bmp']) { + expect(mobileStructuredSendDelivery(accepted('rejected', reason))).toEqual({ + outcome: 'rejected', + operationIdSpent: true, + error: null + }) + } }) - it('shows a provider content rejection verbatim', () => { + it('says a send a Stop withdrew was not sent: no row in the chat says it', () => { expect( - mobileStructuredSendDelivery(accepted('rejected', 'Claude does not support .bmp')) + mobileStructuredSendDelivery(accepted('rejected', 'provider_cancelled_before_start')) ).toEqual({ outcome: 'rejected', operationIdSpent: true, - error: 'Claude does not support .bmp' + error: 'Your message was not sent. Send it again.' }) }) diff --git a/mobile/src/session/mobile-structured-send-delivery.ts b/mobile/src/session/mobile-structured-send-delivery.ts index 466a0d29ff8..55961aead50 100644 --- a/mobile/src/session/mobile-structured-send-delivery.ts +++ b/mobile/src/session/mobile-structured-send-delivery.ts @@ -29,6 +29,7 @@ import type { AgentJournalSubmission } from '../../../src/shared/agent-session-j import type { AgentSessionSendResult } from '../../../src/shared/agent-session-wire' import { agentSessionRefusalOperationState } from '../../../src/shared/agent-session-refusal-retry' import { structuredAgentSessionRejectionNotice } from '../../../src/shared/structured-agent-session-send-disposition' +import { dispatchWasWithdrawn } from '../../../src/shared/structured-agent-session-dispatch-rejection' import type { MobileNativeChatSendOutcome } from './mobile-native-chat-send' import type { StructuredAgentSessionMutationCallResult } from './mobile-structured-agent-session-rpc' @@ -90,7 +91,11 @@ export function mobileStructuredSendDelivery( return { outcome: 'rejected', operationIdSpent: true, - error: structuredAgentSessionRejectionNotice(submission.reason, 'composer-send') + // The host recorded it, so its row in the chat says it was not sent and why; only a Stop's + // withdrawal leaves no row to say it. + error: dispatchWasWithdrawn(submission) + ? structuredAgentSessionRejectionNotice(submission.reason, 'composer-send') + : null } } if (retained) { diff --git a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts index b2464e13e37..14f64df1cce 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.test.ts @@ -15,10 +15,8 @@ import { agentJournalSubmissionKey } from '../../../../shared/agent-session-journal-item-key' import { structuredAgentSessionStartFailureRowIdentity } from '../../../../shared/structured-agent-session-start-failure-row-key' -import { - structuredAgentSessionDeliveryNotices, - structuredAgentSessionStartFailureFacts -} from './structured-agent-session-delivery-notices' +import { structuredAgentSessionStartFailureFacts } from '../../../../shared/structured-agent-session-recorded-rejection-words' +import { structuredAgentSessionDeliveryNotices } from './structured-agent-session-delivery-notices' function entry( clientMessageId: string, diff --git a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts index 912372f1a70..8ec1de07259 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts @@ -14,20 +14,14 @@ // gone) is worded from the journal alone, with no Retry: this client holds nothing to send. import { - readAgentSessionFailureFact, readWholeAgentSessionFailureFact, type AgentSessionFailureFact } from '../../../../shared/agent-session-failure' import { agentJournalSubmissionKey } from '../../../../shared/agent-session-journal-item-key' -import type { - AgentJournalRenderItem, - AgentJournalSubmission -} from '../../../../shared/agent-session-journal-types' +import type { AgentJournalSubmission } from '../../../../shared/agent-session-journal-types' import { agentSessionWriteNotDoneParts } from '../../../../shared/agent-session-refusal-notice' -import { isStructuredAgentSessionStartFailureRow } from '../../../../shared/structured-agent-session-start-failure-row-key' import { structuredAgentSessionEntryIdExpired, - structuredAgentSessionRejectedFailure, type StructuredAgentSessionOutboxEntry } from '../../../../shared/structured-agent-session-outbox' import { dispatchWasWithdrawn } from '../../../../shared/structured-agent-session-dispatch-rejection' @@ -37,56 +31,14 @@ import { } from '../../../../shared/structured-agent-session-outbox-admission' import type { AgentSessionFailureWordsContext } from '../../../../shared/agent-session-failure-words' import { structuredAgentSessionAttemptFailureParts } from '../../../../shared/structured-agent-session-send-disposition' +import { + agentSessionFailureStatedByStartRow, + structuredAgentSessionRecordedRejectionParts +} from '../../../../shared/structured-agent-session-recorded-rejection-words' import { translate } from '@/i18n/i18n' import { agentSessionWriteNoticeText } from './agent-session-write-notice-text' import type { NativeChatDeliveryNotice } from './NativeChatMessageRow' -/** The facts the chat's loaded start-failure rows state. */ -export function structuredAgentSessionStartFailureFacts( - items: readonly AgentJournalRenderItem[] -): AgentSessionFailureFact[] { - const facts: AgentSessionFailureFact[] = [] - for (const item of items) { - if (item.body.kind === 'status' && isStructuredAgentSessionStartFailureRow(item.itemId)) { - const fact = readAgentSessionFailureFact(item.body.failure) - if (fact) { - facts.push(fact) - } - } - } - return facts -} - -/** Whether two facts are one failure: a start's row and the messages it rejected share one. */ -export function sameAgentSessionFailureFact( - a: AgentSessionFailureFact, - b: AgentSessionFailureFact -): boolean { - return ( - a.kind === b.kind && - a.detail?.text === b.detail?.text && - a.detail?.audience === b.detail?.audience && - a.refusal?.code === b.refusal?.code && - a.refusal?.details?.reason === b.refusal?.details?.reason && - a.attachment?.reason === b.attachment?.reason && - a.attachment?.limit === b.attachment?.limit && - a.retry?.error === b.retry?.error && - a.retry?.status === b.retry?.status - ) -} - -/** Whether a loaded start-failure row already states this failure. Matching is identity, not - * wording: what this build can read is enough. */ -export function agentSessionFailureStatedByStartRow( - failure: unknown, - startFailures: readonly AgentSessionFailureFact[] -): boolean { - const fact = readAgentSessionFailureFact(failure) - return ( - fact !== undefined && startFailures.some((stated) => sameAgentSessionFailureFact(stated, fact)) - ) -} - function deliveryNoticeText( entry: StructuredAgentSessionOutboxEntry, context: AgentSessionFailureWordsContext, @@ -132,24 +84,6 @@ function deliveryNoticeText( ) } -/** What a recorded rejection's own row says when no outbox entry here carries it. */ -function recordedRejectionNoticeText( - submission: AgentJournalSubmission, - context: AgentSessionFailureWordsContext, - startFailures: readonly AgentSessionFailureFact[] -): string { - if (agentSessionFailureStatedByStartRow(submission.rejection, startFailures)) { - return agentSessionWriteNoticeText(agentSessionWriteNotDoneParts('send')) - } - return agentSessionWriteNoticeText( - structuredAgentSessionAttemptFailureParts( - structuredAgentSessionRejectedFailure(submission), - context, - readWholeAgentSessionFailureFact(submission.rejection) - ) - ) -} - /** Keyed by the message id the transcript renders each entry under; `agentName` is the chat's * agent, for the words. */ export function structuredAgentSessionDeliveryNotices( @@ -197,10 +131,12 @@ export function structuredAgentSessionDeliveryNotices( const id = agentJournalSubmissionKey(submission.clientMessageId) if (!notices.has(id) && !dispatchWasWithdrawn(submission)) { notices.set(id, { - text: recordedRejectionNoticeText( - submission, - { agentName, retryControl: false }, - startFailures + text: agentSessionWriteNoticeText( + structuredAgentSessionRecordedRejectionParts( + submission, + { agentName, retryControl: false }, + startFailures + ) ) }) } diff --git a/src/renderer/src/components/native-chat/structured-conversation-command-send.ts b/src/renderer/src/components/native-chat/structured-conversation-command-send.ts index 0a351db917f..666715d265b 100644 --- a/src/renderer/src/components/native-chat/structured-conversation-command-send.ts +++ b/src/renderer/src/components/native-chat/structured-conversation-command-send.ts @@ -10,7 +10,7 @@ import { import { agentSessionFailureSentence } from '../../../../shared/agent-session-failure-words' import { translate } from '@/i18n/i18n' import { sayAgentSessionFailureTranslated } from './agent-session-failure-words-text' -import { agentSessionFailureStatedByStartRow } from './structured-agent-session-delivery-notices' +import { agentSessionFailureStatedByStartRow } from '../../../../shared/structured-agent-session-recorded-rejection-words' import type { StructuredAgentSessionWriteOutcome } from './use-structured-agent-session-mutate' export async function sendStructuredConversationCommand(input: { diff --git a/src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-facts.ts b/src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-facts.ts index 83f20103de1..595d68bffa4 100644 --- a/src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-facts.ts +++ b/src/renderer/src/components/native-chat/use-structured-agent-session-start-failure-facts.ts @@ -4,7 +4,7 @@ import type { AgentJournalRenderItem } from '../../../../shared/agent-session-jo import { sameAgentSessionFailureFact, structuredAgentSessionStartFailureFacts -} from './structured-agent-session-delivery-notices' +} from '../../../../shared/structured-agent-session-recorded-rejection-words' const NO_FACTS: readonly AgentSessionFailureFact[] = [] diff --git a/src/renderer/src/components/native-chat/use-structured-agent-session.ts b/src/renderer/src/components/native-chat/use-structured-agent-session.ts index 96ad805560d..0f10c1ded2b 100644 --- a/src/renderer/src/components/native-chat/use-structured-agent-session.ts +++ b/src/renderer/src/components/native-chat/use-structured-agent-session.ts @@ -36,7 +36,7 @@ import { useStructuredAgentSessionContextUsage } from './use-structured-agent-se import { useStructuredAgentSessionRailOutline } from './use-structured-agent-session-rail-outline' import { useStructuredAgentSessionQueuedMessages } from './use-structured-agent-session-queued-messages' import { outboxOutsideQueuedCards } from './structured-agent-session-queued-cards' -import { structuredAgentSessionStartFailureFacts } from './structured-agent-session-delivery-notices' +import { structuredAgentSessionStartFailureFacts } from '../../../../shared/structured-agent-session-recorded-rejection-words' import { hostStatesTurnScopes } from '../../../../shared/native-chat-turn-membership' export type { StructuredPromptItem } from './structured-agent-session-message-projection' diff --git a/src/shared/native-chat-types.ts b/src/shared/native-chat-types.ts index db7b7024cdc..6f0b68243c0 100644 --- a/src/shared/native-chat-types.ts +++ b/src/shared/native-chat-types.ts @@ -212,8 +212,8 @@ export type NativeChatMessage = AgentJournalProducerLinkage & { sentAs?: AgentJournalMessageSendMode /** Accepted but not yet handed to the agent: drawn after everything the agent has done. */ queued?: true - /** Shown as not sent, waiting for the user's Retry: in no turn, so a newer turn's bar and clock - * never land on it, and drawn after the conversation. */ + /** Shown as not sent, as the host recorded it or as this client's outbox holds it: in no turn, so + * a newer turn's bar and clock never land on it. One the journal never placed is drawn last. */ unsent?: true /** Set only by the structured projection, on rows the journal holds, and ranks * them ahead of time. Terminal-backed messages never carry it, and worker reads strip it. */ diff --git a/src/shared/structured-agent-session-recorded-rejection-words.ts b/src/shared/structured-agent-session-recorded-rejection-words.ts new file mode 100644 index 00000000000..dd3dfe724b6 --- /dev/null +++ b/src/shared/structured-agent-session-recorded-rejection-words.ts @@ -0,0 +1,78 @@ +// What a send the host recorded and then did not deliver says on its own row, on every client. +// A rejection that is a failed start's, the fact its loaded row states, says only that it was not +// sent: the row already says why. + +import { + readAgentSessionFailureFact, + readWholeAgentSessionFailureFact, + type AgentSessionFailureFact +} from './agent-session-failure' +import type { AgentSessionFailureWordsContext } from './agent-session-failure-words' +import type { AgentJournalRenderItem, AgentJournalSubmission } from './agent-session-journal-types' +import { agentSessionWriteNotDoneParts } from './agent-session-refusal-notice' +import type { AgentSessionWriteNoticePart } from './agent-session-write-notice-copy' +import { structuredAgentSessionRejectedFailure } from './structured-agent-session-outbox' +import { structuredAgentSessionAttemptFailureParts } from './structured-agent-session-send-disposition' +import { isStructuredAgentSessionStartFailureRow } from './structured-agent-session-start-failure-row-key' + +/** The facts the chat's loaded start-failure rows state. */ +export function structuredAgentSessionStartFailureFacts( + items: readonly AgentJournalRenderItem[] +): AgentSessionFailureFact[] { + const facts: AgentSessionFailureFact[] = [] + for (const item of items) { + if (item.body.kind === 'status' && isStructuredAgentSessionStartFailureRow(item.itemId)) { + const fact = readAgentSessionFailureFact(item.body.failure) + if (fact) { + facts.push(fact) + } + } + } + return facts +} + +/** Whether two facts are one failure: a start's row and the messages it rejected share one. */ +export function sameAgentSessionFailureFact( + a: AgentSessionFailureFact, + b: AgentSessionFailureFact +): boolean { + return ( + a.kind === b.kind && + a.detail?.text === b.detail?.text && + a.detail?.audience === b.detail?.audience && + a.refusal?.code === b.refusal?.code && + a.refusal?.details?.reason === b.refusal?.details?.reason && + a.attachment?.reason === b.attachment?.reason && + a.attachment?.limit === b.attachment?.limit && + a.retry?.error === b.retry?.error && + a.retry?.status === b.retry?.status + ) +} + +/** Whether a loaded start-failure row already states this failure. Matching is identity, not + * wording: what this build can read is enough. */ +export function agentSessionFailureStatedByStartRow( + failure: unknown, + startFailures: readonly AgentSessionFailureFact[] +): boolean { + const fact = readAgentSessionFailureFact(failure) + return ( + fact !== undefined && startFailures.some((stated) => sameAgentSessionFailureFact(stated, fact)) + ) +} + +/** The words for a recorded rejection that no outbox entry on this client carries. */ +export function structuredAgentSessionRecordedRejectionParts( + submission: AgentJournalSubmission, + context: AgentSessionFailureWordsContext, + startFailures: readonly AgentSessionFailureFact[] +): AgentSessionWriteNoticePart[] { + if (agentSessionFailureStatedByStartRow(submission.rejection, startFailures)) { + return agentSessionWriteNotDoneParts('send') + } + return structuredAgentSessionAttemptFailureParts( + structuredAgentSessionRejectedFailure(submission), + context, + readWholeAgentSessionFailureFact(submission.rejection) + ) +} From 64ab6f4d939eca871ae628f86a2bfbc7647a36ac Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:43:44 -0700 Subject: [PATCH 03/23] test(native-chat): a Retry of a recorded rejection leaves the not-sent original in place --- ...ured-agent-session-message-projection.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts index 2b9ba38f897..8f171c57e30 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts @@ -99,6 +99,22 @@ describe('structured agent session message projection', () => { ]) }) + it("keeps the not-sent original when the sender's Retry sends it again as a new message", () => { + const rejected = { ...submission(0), dispatchState: 'rejected' as const, providerItemId: null } + const refusedItem = { ...item(0), itemId: agentJournalSubmissionKey(rejected.clientMessageId) } + const resend = createStructuredAgentSessionOutboxEntry({ + clientMessageId: 'rotated-id', + sessionId: 'session-1', + text: 'send 0', + attachments: [], + queuedAt: 2 + }) + expect(projectStructuredAgentSessionMessages([refusedItem], [resend], [rejected])).toEqual([ + expect.objectContaining({ id: refusedItem.itemId, unsent: true }), + expect.objectContaining({ id: agentJournalSubmissionKey('rotated-id') }) + ]) + }) + it.each([5, 10])('renders %i rapid accepted desktop sends exactly once', (sendCount) => { const outbox = Array.from({ length: sendCount }, (_, index) => createStructuredAgentSessionOutboxEntry({ From 0b9a8a08ddf12f3e272c87cbb14cf0ff34dce0d8 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:02:59 -0700 Subject: [PATCH 04/23] revert(native-chat): keep the older-host recovered-send predicate out of this change It is unrelated to recorded messages staying in the chat; it moves to its own follow-up. --- ...red-agent-session-send-disposition.test.ts | 27 ----------- ...ructured-agent-session-send-disposition.ts | 3 +- ...-agent-session-unanswered-dispatch.test.ts | 48 ------------------- ...tured-agent-session-unanswered-dispatch.ts | 17 ++----- 4 files changed, 4 insertions(+), 91 deletions(-) delete mode 100644 src/shared/structured-agent-session-unanswered-dispatch.test.ts diff --git a/src/shared/structured-agent-session-send-disposition.test.ts b/src/shared/structured-agent-session-send-disposition.test.ts index 5514d422015..98ea2ff8b67 100644 --- a/src/shared/structured-agent-session-send-disposition.test.ts +++ b/src/shared/structured-agent-session-send-disposition.test.ts @@ -411,33 +411,6 @@ describe('ambiguous operation refusals', () => { expect(disposition.entries).toMatchObject([{ clientMessageId: 'fresh-id', state: 'rejected' }]) }) - it("parks an older host's restart answer that carries the reason but not the recovered marker", () => { - const result = rejectedWith(null) - if (!result.ok || !('submission' in result.value)) { - throw new Error('expected a send result') - } - result.value.submission = { - ...result.value.submission, - dispatchState: 'unknown', - reason: 'host_restarted_before_acknowledgement' - } - - const disposition = disposeStructuredAgentSessionSendResult({ - entries: [entry], - entry, - result, - createOperationId: () => 'unused' - }) - - expect(disposition.entries).toMatchObject([ - { - clientMessageId: entry.clientMessageId, - state: 'unconfirmed', - retryAfterUnknownSubmittedAt: -1 - } - ]) - }) - it('parks a recovered missing submission without polling forever', () => { const result = rejectedWith(null) if (!result.ok || !('submission' in result.value)) { diff --git a/src/shared/structured-agent-session-send-disposition.ts b/src/shared/structured-agent-session-send-disposition.ts index a8f68057fd5..f717e54f2d4 100644 --- a/src/shared/structured-agent-session-send-disposition.ts +++ b/src/shared/structured-agent-session-send-disposition.ts @@ -22,7 +22,6 @@ import { import type { AgentSessionFailureFact } from './agent-session-failure' import type { AgentSessionFailureWordsContext } from './agent-session-failure-words' import { classifyDispatchRejection } from './structured-agent-session-dispatch-rejection' -import { isRecoveredStructuredAgentSessionSubmission } from './structured-agent-session-unanswered-dispatch' import { classifyStructuredAgentSessionSendFailure, requeueStructuredAgentSessionSendRefusal, @@ -278,7 +277,7 @@ export function disposeStructuredAgentSessionSendResult( error: null } } - if (isRecoveredStructuredAgentSessionSubmission(submission)) { + if (submission.dispatchState === 'unknown' && submission.recovered) { return { entries: input.entries.map((candidate) => candidate.clientMessageId === input.entry.clientMessageId diff --git a/src/shared/structured-agent-session-unanswered-dispatch.test.ts b/src/shared/structured-agent-session-unanswered-dispatch.test.ts deleted file mode 100644 index 39a67f880e7..00000000000 --- a/src/shared/structured-agent-session-unanswered-dispatch.test.ts +++ /dev/null @@ -1,48 +0,0 @@ -import { describe, expect, it } from 'vitest' -import type { AgentJournalSubmission } from './agent-session-journal-types' -import { - isRecoveredStructuredAgentSessionSubmission, - isUnansweredStructuredAgentSessionDispatch -} from './structured-agent-session-unanswered-dispatch' - -function submission(patch: Partial): AgentJournalSubmission { - return { - clientMessageId: 'client-1', - fence: 1, - payloadFingerprint: 'fingerprint', - dispatchState: 'unknown', - providerItemId: null, - reason: null, - submittedAt: 1, - resolvedAt: 1, - ...patch - } -} - -describe('a send whose outcome the host lost for good', () => { - it('is one the host marked recovered', () => { - const row = submission({ recovered: true, reason: 'provider_exited_before_acknowledgement' }) - expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(true) - expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(false) - }) - - it("is an older host's restart row, which carries the reason without the marker", () => { - const row = submission({ reason: 'host_restarted_before_acknowledgement' }) - expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(true) - expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(false) - }) - - it('is never a live unknown, which something still running may answer', () => { - const row = submission({ reason: 'provider_ack_ambiguous' }) - expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(false) - expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(true) - }) - - it('is never a settled send, whatever its reason says', () => { - expect( - isRecoveredStructuredAgentSessionSubmission( - submission({ dispatchState: 'rejected', reason: 'host_restarted_before_acknowledgement' }) - ) - ).toBe(false) - }) -}) diff --git a/src/shared/structured-agent-session-unanswered-dispatch.ts b/src/shared/structured-agent-session-unanswered-dispatch.ts index 2113525564d..412b3e98196 100644 --- a/src/shared/structured-agent-session-unanswered-dispatch.ts +++ b/src/shared/structured-agent-session-unanswered-dispatch.ts @@ -1,19 +1,6 @@ import type { AgentJournalSubmission } from './agent-session-journal-types' import { isQueuedAgentJournalSubmission } from './agent-session-queued-submission' -/** A send whose outcome the host lost for good when the process that sent it went away (a restart, - * a provider exit, an idle release): nothing still running can answer it. */ -export function isRecoveredStructuredAgentSessionSubmission( - submission: Pick -): boolean { - return ( - submission.dispatchState === 'unknown' && - (submission.recovered === true || - // Older hosts publish the recovery reason but omit the optional marker. - submission.reason === 'host_restarted_before_acknowledgement') - ) -} - /** One send the provider has neither opened a turn for nor refused; the rule is explained on * `hasUnansweredStructuredAgentSessionDispatch`, which asks it of every send. */ export function isUnansweredStructuredAgentSessionDispatch( @@ -28,7 +15,9 @@ export function isUnansweredStructuredAgentSessionDispatch( (currentFence == null || submission.fence >= currentFence) && (submission.dispatchState === 'pending' || (submission.dispatchState === 'unknown' && - !isRecoveredStructuredAgentSessionSubmission(submission))) + submission.recovered !== true && + // Older hosts publish the recovery reason but omit the optional marker. + submission.reason !== 'host_restarted_before_acknowledgement')) ) } From 412ab128bc507fc8c598b7799ff0b1aea824c4c1 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:07:13 -0700 Subject: [PATCH 05/23] fix(native-chat): the desktop words a not-sent message it holds no outbox entry for The pane read the journal's rejection rows only while its own outbox held a rejected entry, so another window, or the sender after its entry was gone, drew the host's not-sent message as a plain bubble with no line under it. It now reads them whenever a row is shown as not sent. --- ...turedSession.start-failure-notice.test.tsx | 52 ++++++++++++++++++- .../NativeChatStructuredSession.tsx | 5 +- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/renderer/src/components/native-chat/NativeChatStructuredSession.start-failure-notice.test.tsx b/src/renderer/src/components/native-chat/NativeChatStructuredSession.start-failure-notice.test.tsx index 6f529c1d3c9..762f0f90989 100644 --- a/src/renderer/src/components/native-chat/NativeChatStructuredSession.start-failure-notice.test.tsx +++ b/src/renderer/src/components/native-chat/NativeChatStructuredSession.start-failure-notice.test.tsx @@ -8,7 +8,11 @@ import { agentJournalItemKey, agentJournalSubmissionKey } from '../../../../shared/agent-session-journal-item-key' -import type { AgentJournalRenderItem } from '../../../../shared/agent-session-journal-types' +import type { + AgentJournalRenderItem, + AgentJournalSubmission +} from '../../../../shared/agent-session-journal-types' +import { projectStructuredAgentSessionMessages } from '../../../../shared/structured-agent-session-message-projection' import { structuredAgentSessionStartFailureRowIdentity } from '../../../../shared/structured-agent-session-start-failure-row-key' const { mocks, moduleFactories, resetStructuredSessionMocks } = await vi.hoisted(async () => @@ -78,7 +82,7 @@ function rejected(clientMessageId: string, reason: string, rejection: AgentSessi rejection, submittedAt: 1, resolvedAt: 1 - } + } satisfies AgentJournalSubmission } } @@ -161,3 +165,47 @@ it("keeps the start failure's own words when its row is not loaded", async () => within(await notice('first')).getByText('Claude stopped before it finished starting.') ).toBeTruthy() }) + +// Another window, or the sender after its entry is gone: the host's record alone says it. +it('words a recorded rejection this window holds no outbox entry for, with no Retry', async () => { + const userRow = (clientMessageId: string, sequence: number): AgentJournalRenderItem => ({ + itemId: agentJournalSubmissionKey(clientMessageId), + revision: 1, + sequence, + observedAt: sequence, + body: { kind: 'message', role: 'user', blocks: [{ type: 'text', text: clientMessageId }] } + }) + const providerRejected: AgentSessionFailureFact = { + kind: 'providerRejected', + detail: { text: 'Image type .bmp', audience: 'person' } + } + mocks.journalItems = [startFailureRow(START_FAILED), userRow('stated', 2), userRow('other', 3)] + const submissions = [ + rejected('stated', START_FAILED_REASON, START_FAILED).submission, + rejected( + 'other', + 'The provider did not accept this message: Image type .bmp.', + providerRejected + ).submission + ] + mocks.submissions = submissions + mocks.messages = projectStructuredAgentSessionMessages(mocks.journalItems, [], submissions) + render( + + ) + + expect(within(await notice('stated')).getByText('Your message was not sent.')).toBeTruthy() + expect( + within(await notice('other')).getByText( + 'The provider did not accept this message: Image type .bmp.' + ) + ).toBeTruthy() + expect(screen.queryByRole('button', { name: 'Retry' })).toBeNull() +}) diff --git a/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx b/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx index 6b79c8a7fc8..48e24f4d464 100644 --- a/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx +++ b/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx @@ -122,8 +122,9 @@ export function NativeChatStructuredSession( retryRef.current(clientMessageId) }, []) const agentLabel = structuredAgentLabel(props.agent === 'codex' ? 'codex' : 'claude') - // Only a rejected message reads the journal's rows, so a new batch of them re-renders no row else. - const hasRejected = controller.outbox.some((entry) => entry.state === 'rejected') + // Only a row shown as not sent reads the journal's rows, so a new batch of them re-renders no row + // else. Read from the transcript, not this window's outbox: the host's record alone shows one. + const hasRejected = controller.messages.some((message) => message.unsent === true) const rejectionRows = hasRejected ? controller.submissions : NO_SUBMISSIONS const startFailures = useStructuredAgentSessionStartFailureFacts( controller.journalItems, From c3ce6bcd7f36b0e70072a7487ccc09e990d0d792 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:08:39 -0700 Subject: [PATCH 06/23] fix(native-chat): a failed /compact or queued draft is said once, where it already is A rejected /compact drew as a not-sent "/compact" message while its reply or its turn's "Compaction failed" row already said it failed, so the failure showed twice. A rejected hand-off of a queued draft drew as a not-sent bubble while the draft's card kept the same text. Both stay hidden as before: the command entry is read from the journal's own item, the hand-off from its queued-draft link. --- ...d-agent-session-message-projection.test.ts | 131 +++++++++++++++++- ...ctured-agent-session-message-projection.ts | 15 +- 2 files changed, 139 insertions(+), 7 deletions(-) diff --git a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts index 8f171c57e30..27a0d68584c 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts @@ -3,7 +3,14 @@ import type { AgentJournalRenderItem, AgentJournalSubmission } from '../../../../shared/agent-session-journal-types' -import { agentJournalSubmissionKey } from '../../../../shared/agent-session-journal-item-key' +import type { AgentSessionFailureFact } from '../../../../shared/agent-session-failure' +import { agentSessionFailureWords } from '../../../../shared/agent-session-failure-words' +import { + agentJournalItemKey, + agentJournalSubmissionKey +} from '../../../../shared/agent-session-journal-item-key' +import { agentJournalTurnBody } from '../../../../shared/agent-session-turn-record' +import { projectStructuredAgentSessionMessages as projectForPhone } from '../../../../shared/structured-agent-session-message-projection' import { createStructuredAgentSessionOutboxEntry } from '../../../../shared/structured-agent-session-outbox' import { projectStructuredAgentSessionMessages } from './structured-agent-session-message-projection' @@ -67,7 +74,7 @@ describe('structured agent session message projection', () => { ]) }) - it('leaves out a send a Stop withdrew: its text went back to its sender', () => { + it('leaves out a send the user withdrew with Stop', () => { const withdrawn = { ...submission(0), dispatchState: 'rejected' as const, @@ -115,6 +122,126 @@ describe('structured agent session message projection', () => { ]) }) + // The command's reply (blocked) or its turn's result row (refused) already says it failed. + const busy = { text: 'thread busy', audience: 'person' as const } + it.each<{ + name: string + rejection: AgentSessionFailureFact + resultRow?: AgentSessionFailureFact + }>([ + { name: 'blocked at handover', rejection: { kind: 'commandRefused' } }, + { + name: 'refused by the provider', + rejection: { kind: 'providerRejected', detail: busy }, + resultRow: { kind: 'compactionFailed', detail: busy } + } + ])( + 'says a /compact $name failed only where the command reports it', + ({ rejection, resultRow }) => { + const opensTurn = resultRow !== undefined + const compact = { + ...submission(0), + dispatchState: 'rejected' as const, + providerItemId: null, + rejection + } + const userItemId = agentJournalSubmissionKey(compact.clientMessageId) + const commandItem: AgentJournalRenderItem = { + itemId: userItemId, + revision: 1, + sequence: 0, + observedAt: 0, + body: { + kind: 'message', + role: 'user', + blocks: [{ type: 'text', text: '/compact' }], + command: { name: 'compact' } + } + } + const turnItemId = agentJournalItemKey({ + provider: 'orca', + clientMessageId: 'command-turn:c' + }) + const resultId = agentJournalItemKey({ + provider: 'orca', + clientMessageId: 'command-result:c' + }) + const turnRows: AgentJournalRenderItem[] = resultRow + ? [ + { + itemId: turnItemId, + revision: 1, + sequence: 1, + observedAt: 1, + body: agentJournalTurnBody({ + turnId: 'compact:c', + state: 'completed', + outcome: 'failure', + userItemId, + requestedAt: 0, + startedAt: 1, + completedAt: 2 + }) + }, + { + itemId: resultId, + revision: 1, + sequence: 2, + observedAt: 2, + turnScope: { kind: 'turn', turnItemId }, + body: { + kind: 'status', + tone: 'error', + ...agentSessionFailureWords(resultRow, { agentName: 'Codex', surface: 'row' }) + } + } + ] + : [] + const items = [commandItem, ...turnRows] + for (const messages of [ + projectStructuredAgentSessionMessages(items, [], [compact]), + projectForPhone(items, [], [compact]) + ]) { + expect(messages.find((message) => message.id === userItemId)).toBeUndefined() + expect(messages.some((message) => message.unsent === true)).toBe(false) + expect(messages.some((message) => message.id === resultId)).toBe(opensTurn) + } + } + ) + + // The host sends a queued draft under a fresh id per hand-off; its card keeps the text meanwhile. + it('leaves a rejected queued-draft hand-off to its card: one bubble once a later hand-off lands', () => { + const handOff = (index: number, dispatchState: 'rejected' | 'accepted') => ({ + ...submission(index), + clientMessageId: `handoff-${index}`, + queuedMessageId: 'draft-1', + dispatchState, + ...(dispatchState === 'rejected' + ? { + providerItemId: null, + reason: 'host_restarted', + rejection: { kind: 'hostRestarted' as const } + } + : {}) + }) + const handOffRow = (index: number): AgentJournalRenderItem => ({ + ...item(index), + itemId: agentJournalSubmissionKey(`handoff-${index}`), + body: { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'same draft text' }] } + }) + const items = [handOffRow(1), handOffRow(2)] + const submissions = [handOff(1, 'rejected'), handOff(2, 'accepted')] + for (const messages of [ + projectStructuredAgentSessionMessages(items, [], submissions), + projectForPhone(items, [], submissions) + ]) { + expect(messages).toEqual([ + expect.objectContaining({ id: agentJournalSubmissionKey('handoff-2') }) + ]) + expect(messages[0]?.unsent).toBeUndefined() + } + }) + it.each([5, 10])('renders %i rapid accepted desktop sends exactly once', (sendCount) => { const outbox = Array.from({ length: sendCount }, (_, index) => createStructuredAgentSessionOutboxEntry({ diff --git a/src/shared/structured-agent-session-message-projection.ts b/src/shared/structured-agent-session-message-projection.ts index 5223b4c8485..7de8a923efd 100644 --- a/src/shared/structured-agent-session-message-projection.ts +++ b/src/shared/structured-agent-session-message-projection.ts @@ -8,6 +8,7 @@ import type { StructuredAgentSessionOutboxEntry } from './structured-agent-sessi import { structuredAgentSessionEntryHeldForRetry } from './structured-agent-session-outbox-admission' import { reconcileStructuredAgentSessionOutboxWithQueue } from './structured-agent-session-draft-hand-off' import { dispatchWasWithdrawn } from './structured-agent-session-dispatch-rejection' +import { isStructuredAgentSessionCommandEntry } from './structured-agent-session-command-entry' import { projectStructuredItemsToNativeChat } from './structured-agent-session-projection' export function projectStructuredAgentSessionMessages( @@ -19,16 +20,17 @@ export function projectStructuredAgentSessionMessages( const optimistic = reconcileStructuredAgentSessionOutboxWithQueue(outbox, submissions) // A send the host recorded and then did not deliver stays in the conversation from the host's // record, shown as not sent, for every viewer: dropping the sender's outbox entry can never make - // it vanish. Only a Stop's withdrawal leaves it, since its text went back to its sender. + // it vanish. It leaves only where something else owns it: the user withdrew it with Stop, its + // queued draft's card keeps the text, or it is a command whose reply or result row says it failed. const notSent = new Set() - const withdrawn = new Set() + const ownedElsewhere = new Set() for (const submission of submissions) { if (submission.dispatchState !== 'rejected') { continue } const key = agentJournalSubmissionKey(submission.clientMessageId) - if (dispatchWasWithdrawn(submission)) { - withdrawn.add(key) + if (dispatchWasWithdrawn(submission) || submission.queuedMessageId !== undefined) { + ownedElsewhere.add(key) } else { notSent.add(key) } @@ -36,7 +38,10 @@ export function projectStructuredAgentSessionMessages( const visibleItems: AgentJournalRenderItem[] = [] const refused = new Map() for (const item of items) { - if (withdrawn.has(item.itemId)) { + if ( + ownedElsewhere.has(item.itemId) || + (notSent.has(item.itemId) && isStructuredAgentSessionCommandEntry(item.body)) + ) { refused.set(item.itemId, item) } else { visibleItems.push(item) From 71e43557bfb38cbd403bdccd5746d02c574f3c95 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:09:35 -0700 Subject: [PATCH 07/23] fix(native-chat): the rail stays lit while the reader is on a not-sent message A message shown as not sent has no rail tick, but the active-tick rule still lit its own id, so the rail lit nothing while it was the row being read, for example at the bottom of a chat ending in one. The tick before it stays lit. --- .../native-chat-active-rail-item.test.ts | 41 ++++++++++++++++++- .../native-chat-active-rail-item.ts | 18 +++++--- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/renderer/src/components/native-chat/native-chat-active-rail-item.test.ts b/src/renderer/src/components/native-chat/native-chat-active-rail-item.test.ts index 72e4898ce04..ee55329caa6 100644 --- a/src/renderer/src/components/native-chat/native-chat-active-rail-item.test.ts +++ b/src/renderer/src/components/native-chat/native-chat-active-rail-item.test.ts @@ -123,7 +123,46 @@ describe('active rail item', () => { ).toBe('u2') }) - // A rejected send the journal recorded keeps its place but opens no turn. + // A send shown as not sent keeps its place but has no tick, as the rail's items agree. + it('keeps the tick before a not-sent row lit, never the row itself', () => { + const slots: NativeChatRailSlot[] = [ + ...TURNS, + { turnKey: undefined, message: { id: 'not-sent', role: 'user', unsent: true } }, + { turnKey: undefined, message: { id: 'not-sent-2', role: 'user', unsent: true } } + ] + expect( + findActiveNativeChatRailItem({ + slots, + virtualItems: rows(12), + scrollTop: 900, + clientHeight: VIEWPORT, + scrollHeight: 1200, + previousActiveId: null + }) + ).toBe('u2') + expect( + findActiveNativeChatRailItem({ + slots, + virtualItems: rows(12), + scrollTop: 1010, + ...MID_SCROLL, + scrollHeight: 2000 + }) + ).toBe('u2') + expect( + findActiveNativeChatRailItem({ + slots: [ + { turnKey: undefined, message: { id: 'first', role: 'user', unsent: true } }, + ...TURNS + ], + virtualItems: rows(11), + scrollTop: 50, + ...MID_SCROLL, + scrollHeight: 1100 + }) + ).toBeNull() + }) + it('lights a prompt in no turn by its own id', () => { const slots: NativeChatRailSlot[] = [ ...TURNS, diff --git a/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts b/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts index 5ca6c158466..9647a55d48f 100644 --- a/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts +++ b/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts @@ -27,11 +27,17 @@ export type NativeChatRailVirtualItem = { /** Only the fields the rail reads, so a test needs no slot builder. */ export type NativeChatRailSlot = { turnKey: string | undefined - /** A user row in no turn (one shown as not sent) lights its own tick. */ - message?: { id: string; role: string } + /** A user row in no turn lights its own tick. */ + message?: { id: string; role: string; unsent?: true } } -function railTickOf(slot: NativeChatRailSlot | undefined): string | null { +/** A row shown as not sent has no tick and no turn, so the tick before it stays lit. */ +function railTickAt(slots: readonly NativeChatRailSlot[], index: number): string | null { + let at = index + while (slots[at]?.message?.unsent === true) { + at -= 1 + } + const slot = slots[at] return slot?.turnKey ?? (slot?.message?.role === 'user' ? slot.message.id : null) } @@ -59,7 +65,7 @@ export function findActiveNativeChatRailItem({ const atBottom = scrollHeight - clientHeight - scrollTop <= NATIVE_CHAT_BOTTOM_THRESHOLD_PX if (atBottom) { const last = virtualItems.at(-1) - return last === undefined ? previousActiveId : railTickOf(slots[last.index]) + return last === undefined ? previousActiveId : railTickAt(slots, last.index) } let fold: NativeChatRailVirtualItem | undefined @@ -71,12 +77,12 @@ export function findActiveNativeChatRailItem({ // Scrolled above everything the window holds: the first windowed row is the // nearest thing to the fold. if (fold === undefined) { - return railTickOf(slots[virtualItems[0]?.index ?? -1]) + return railTickAt(slots, virtualItems[0]?.index ?? -1) } // The window lags the scroll by a commit, so a fold past every row it holds is // a stale read, not an answer. Holding the previous tick beats blanking one. if (fold.end <= scrollTop) { return previousActiveId } - return railTickOf(slots[fold.index]) + return railTickAt(slots, fold.index) } From d9e060543fef9f4a18c1e9f19c05835d86ccba6c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:22:22 -0700 Subject: [PATCH 08/23] docs(native-chat): say a not-sent row lights no tick of its own --- .../src/components/native-chat/native-chat-active-rail-item.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts b/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts index 9647a55d48f..fc45d2d1d10 100644 --- a/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts +++ b/src/renderer/src/components/native-chat/native-chat-active-rail-item.ts @@ -27,7 +27,7 @@ export type NativeChatRailVirtualItem = { /** Only the fields the rail reads, so a test needs no slot builder. */ export type NativeChatRailSlot = { turnKey: string | undefined - /** A user row in no turn lights its own tick. */ + /** A user row in no turn lights its own tick, unless it is shown as not sent. */ message?: { id: string; role: string; unsent?: true } } From 87f2c0f92ad27c49b71807ce07f1e48aa4066e0b Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 3 Oct 2026 22:46:54 -0700 Subject: [PATCH 09/23] fix(native-chat): read an older host's restart answer as recovered when sending, too Hosts from before the recovered marker publish only the restart reason. The working-state rule already read that reason as recovered; the send answer now uses the same predicate, so both agree on those hosts. --- ...red-agent-session-send-disposition.test.ts | 27 +++++++++++ ...ructured-agent-session-send-disposition.ts | 3 +- ...-agent-session-unanswered-dispatch.test.ts | 48 +++++++++++++++++++ ...tured-agent-session-unanswered-dispatch.ts | 17 +++++-- 4 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 src/shared/structured-agent-session-unanswered-dispatch.test.ts diff --git a/src/shared/structured-agent-session-send-disposition.test.ts b/src/shared/structured-agent-session-send-disposition.test.ts index 98ea2ff8b67..5514d422015 100644 --- a/src/shared/structured-agent-session-send-disposition.test.ts +++ b/src/shared/structured-agent-session-send-disposition.test.ts @@ -411,6 +411,33 @@ describe('ambiguous operation refusals', () => { expect(disposition.entries).toMatchObject([{ clientMessageId: 'fresh-id', state: 'rejected' }]) }) + it("parks an older host's restart answer that carries the reason but not the recovered marker", () => { + const result = rejectedWith(null) + if (!result.ok || !('submission' in result.value)) { + throw new Error('expected a send result') + } + result.value.submission = { + ...result.value.submission, + dispatchState: 'unknown', + reason: 'host_restarted_before_acknowledgement' + } + + const disposition = disposeStructuredAgentSessionSendResult({ + entries: [entry], + entry, + result, + createOperationId: () => 'unused' + }) + + expect(disposition.entries).toMatchObject([ + { + clientMessageId: entry.clientMessageId, + state: 'unconfirmed', + retryAfterUnknownSubmittedAt: -1 + } + ]) + }) + it('parks a recovered missing submission without polling forever', () => { const result = rejectedWith(null) if (!result.ok || !('submission' in result.value)) { diff --git a/src/shared/structured-agent-session-send-disposition.ts b/src/shared/structured-agent-session-send-disposition.ts index f717e54f2d4..a8f68057fd5 100644 --- a/src/shared/structured-agent-session-send-disposition.ts +++ b/src/shared/structured-agent-session-send-disposition.ts @@ -22,6 +22,7 @@ import { import type { AgentSessionFailureFact } from './agent-session-failure' import type { AgentSessionFailureWordsContext } from './agent-session-failure-words' import { classifyDispatchRejection } from './structured-agent-session-dispatch-rejection' +import { isRecoveredStructuredAgentSessionSubmission } from './structured-agent-session-unanswered-dispatch' import { classifyStructuredAgentSessionSendFailure, requeueStructuredAgentSessionSendRefusal, @@ -277,7 +278,7 @@ export function disposeStructuredAgentSessionSendResult( error: null } } - if (submission.dispatchState === 'unknown' && submission.recovered) { + if (isRecoveredStructuredAgentSessionSubmission(submission)) { return { entries: input.entries.map((candidate) => candidate.clientMessageId === input.entry.clientMessageId diff --git a/src/shared/structured-agent-session-unanswered-dispatch.test.ts b/src/shared/structured-agent-session-unanswered-dispatch.test.ts new file mode 100644 index 00000000000..39a67f880e7 --- /dev/null +++ b/src/shared/structured-agent-session-unanswered-dispatch.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from 'vitest' +import type { AgentJournalSubmission } from './agent-session-journal-types' +import { + isRecoveredStructuredAgentSessionSubmission, + isUnansweredStructuredAgentSessionDispatch +} from './structured-agent-session-unanswered-dispatch' + +function submission(patch: Partial): AgentJournalSubmission { + return { + clientMessageId: 'client-1', + fence: 1, + payloadFingerprint: 'fingerprint', + dispatchState: 'unknown', + providerItemId: null, + reason: null, + submittedAt: 1, + resolvedAt: 1, + ...patch + } +} + +describe('a send whose outcome the host lost for good', () => { + it('is one the host marked recovered', () => { + const row = submission({ recovered: true, reason: 'provider_exited_before_acknowledgement' }) + expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(true) + expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(false) + }) + + it("is an older host's restart row, which carries the reason without the marker", () => { + const row = submission({ reason: 'host_restarted_before_acknowledgement' }) + expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(true) + expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(false) + }) + + it('is never a live unknown, which something still running may answer', () => { + const row = submission({ reason: 'provider_ack_ambiguous' }) + expect(isRecoveredStructuredAgentSessionSubmission(row)).toBe(false) + expect(isUnansweredStructuredAgentSessionDispatch(row)).toBe(true) + }) + + it('is never a settled send, whatever its reason says', () => { + expect( + isRecoveredStructuredAgentSessionSubmission( + submission({ dispatchState: 'rejected', reason: 'host_restarted_before_acknowledgement' }) + ) + ).toBe(false) + }) +}) diff --git a/src/shared/structured-agent-session-unanswered-dispatch.ts b/src/shared/structured-agent-session-unanswered-dispatch.ts index 412b3e98196..2113525564d 100644 --- a/src/shared/structured-agent-session-unanswered-dispatch.ts +++ b/src/shared/structured-agent-session-unanswered-dispatch.ts @@ -1,6 +1,19 @@ import type { AgentJournalSubmission } from './agent-session-journal-types' import { isQueuedAgentJournalSubmission } from './agent-session-queued-submission' +/** A send whose outcome the host lost for good when the process that sent it went away (a restart, + * a provider exit, an idle release): nothing still running can answer it. */ +export function isRecoveredStructuredAgentSessionSubmission( + submission: Pick +): boolean { + return ( + submission.dispatchState === 'unknown' && + (submission.recovered === true || + // Older hosts publish the recovery reason but omit the optional marker. + submission.reason === 'host_restarted_before_acknowledgement') + ) +} + /** One send the provider has neither opened a turn for nor refused; the rule is explained on * `hasUnansweredStructuredAgentSessionDispatch`, which asks it of every send. */ export function isUnansweredStructuredAgentSessionDispatch( @@ -15,9 +28,7 @@ export function isUnansweredStructuredAgentSessionDispatch( (currentFence == null || submission.fence >= currentFence) && (submission.dispatchState === 'pending' || (submission.dispatchState === 'unknown' && - submission.recovered !== true && - // Older hosts publish the recovery reason but omit the optional marker. - submission.reason !== 'host_restarted_before_acknowledgement')) + !isRecoveredStructuredAgentSessionSubmission(submission))) ) } From 3cc5e1dac659ffd163b81b28261cd843261f4ca5 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 3 Oct 2026 22:47:26 -0700 Subject: [PATCH 10/23] Avoid duplicate ripgrep scans for full file inventories (#23490) * Avoid duplicate ripgrep scans for unbounded file inventories * Pin the broad ripgrep pass superset contract --- src/main/ipc/filesystem-list-files.test.ts | 155 +++++++----------- src/main/ipc/filesystem-list-files.ts | 13 +- .../fs-handler-list-files-cancel.test.ts | 20 +-- .../fs-handler-list-files-ignored.test.ts | 91 +++++----- src/relay/fs-handler-list-files.ts | 27 +-- src/shared/quick-open-filter.test.ts | 13 ++ src/shared/quick-open-filter.ts | 2 +- 7 files changed, 132 insertions(+), 189 deletions(-) diff --git a/src/main/ipc/filesystem-list-files.test.ts b/src/main/ipc/filesystem-list-files.test.ts index e9a4d406d7f..84567c85419 100644 --- a/src/main/ipc/filesystem-list-files.test.ts +++ b/src/main/ipc/filesystem-list-files.test.ts @@ -136,38 +136,60 @@ describe('filesystem-list-files', () => { expect(spawnMock.mock.calls[0]?.[1]).not.toContain('--version') }) - it('merges normal files and ignored files and filters correctly', async () => { - const p1 = createMockProcess() - const p2 = createMockProcess() + it('keeps source files first when only a serialized byte budget is provided', async () => { + const source = createMockProcess() + const broad = createMockProcess() + spawnMock.mockImplementation((_command, args: string[]) => + isIgnoredRgPass(args) ? broad : source + ) + const listing = listQuickOpenFiles( + '/mock/root', + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: authorization and workspace lookup are mocked above. + {} as Store, + undefined, + undefined, + undefined, + 20 + ) + await flushMicrotasks() + expect(spawnMock).toHaveBeenCalledTimes(1) + expect(spawnMock.mock.calls[0]?.[1]).not.toContain('--no-ignore-vcs') + source.stdout?.emit('data', 'source.ts\n') + source.emit('close', 0, null) + await flushMicrotasks() + expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock.mock.calls[1]?.[1]).toContain('--no-ignore-vcs') + broad.stdout?.emit('data', 'ignored-file.ts\n') + await expect(listing).resolves.toEqual(['source.ts']) + expect(broad.kill).toHaveBeenCalledOnce() + }) - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + it('lists normal and ignored files with one broad scan and filters correctly', async () => { + const p1 = createMockProcess() + + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('/mock/root', storeMock) await flushMicrotasks() - expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(1) + expect(spawnMock.mock.calls[0]?.[1]).toContain('--no-ignore-vcs') // Simulate stdout output for normal files setTimeout(() => { - ;(p1.stdout as unknown as EventEmitter).emit('data', 'file1.ts\n') - ;(p1.stdout as unknown as EventEmitter).emit('data', 'node_modules/bad.js\n') - ;(p1.stdout as unknown as EventEmitter).emit('data', '.git/config\n') - ;(p1.stdout as unknown as EventEmitter).emit('data', '.github/workflows/ci.yml\n') - ;(p1.stdout as unknown as EventEmitter).emit('data', 'dir1/') // incomplete line - ;(p1.stdout as unknown as EventEmitter).emit('data', 'file2.js\n') - p1.emit('close', 0, null) + p1.stdout?.emit('data', 'file1.ts\n') + p1.stdout?.emit('data', 'node_modules/bad.js\n') + p1.stdout?.emit('data', '.git/config\n') + p1.stdout?.emit('data', '.github/workflows/ci.yml\n') + p1.stdout?.emit('data', 'dir1/') // incomplete line + p1.stdout?.emit('data', 'file2.js\n') - // Simulate stdout output for ignored files - ;(p2.stdout as unknown as EventEmitter).emit('data', '.env.local\n') - ;(p2.stdout as unknown as EventEmitter).emit('data', 'dist/generated.js\n') - ;(p2.stdout as unknown as EventEmitter).emit('data', 'file1.ts\n') // Duplicate - ;(p2.stdout as unknown as EventEmitter).emit('data', 'node_modules/ignored.js\n') - p2.emit('close', 0, null) + // The broad pass includes ignored files too. + p1.stdout?.emit('data', '.env.local\n') + p1.stdout?.emit('data', 'dist/generated.js\n') + p1.stdout?.emit('data', 'file1.ts\n') // Duplicate + p1.stdout?.emit('data', 'node_modules/ignored.js\n') + p1.emit('close', 0, null) }, 10) const result = await promise @@ -183,15 +205,9 @@ describe('filesystem-list-files', () => { it('spawns the bundled Linux rg inside the registered WSL runtime for Windows-path worktrees', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() getLocalGitOptionsForRegisteredWorktreeMock.mockReturnValue({ wslDistro: 'Ubuntu' }) - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('C:\\repo', storeMock) @@ -199,7 +215,6 @@ describe('filesystem-list-files', () => { setTimeout(() => { ;(p1.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') p1.emit('close', 0, null) - p2.emit('close', 0, null) }, 10) await expect(promise).resolves.toEqual(['src/index.ts']) @@ -214,15 +229,9 @@ describe('filesystem-list-files', () => { it('normalizes absolute WSL rg output for Windows-path worktrees', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() getLocalGitOptionsForRegisteredWorktreeMock.mockReturnValue({ wslDistro: 'Ubuntu' }) - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('C:\\repo', storeMock) @@ -230,7 +239,6 @@ describe('filesystem-list-files', () => { setTimeout(() => { ;(p1.stdout as unknown as EventEmitter).emit('data', '/mnt/c/repo/src/index.ts\n') p1.emit('close', 0, null) - p2.emit('close', 0, null) }, 10) await expect(promise).resolves.toEqual(['src/index.ts']) @@ -238,16 +246,13 @@ describe('filesystem-list-files', () => { it('treats a WSL launcher exit 127 as the bundled rg failing to start', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() Object.defineProperty(p1, 'pid', { value: 1 }) - Object.defineProperty(p2, 'pid', { value: 2 }) getLocalGitOptionsForRegisteredWorktreeMock.mockReturnValue({ wslDistro: 'Ubuntu' }) - spawnMock.mockImplementation((_cmd, args: string[]) => (isIgnoredRgPass(args) ? p2 : p1)) + spawnMock.mockReturnValue(p1) const promise = listQuickOpenFiles('C:\\repo', {} as unknown as Store) setTimeout(() => { p1.emit('close', 127, null) - p2.emit('close', 0, null) }, 0) await expect(promise).rejects.toThrow(BUNDLED_ERROR) @@ -256,48 +261,33 @@ describe('filesystem-list-files', () => { it("rejects with the bundled-ripgrep error when a native launcher exits outside ripgrep's contract", async () => { const p1 = createMockProcess() - const p2 = createMockProcess() - spawnMock.mockImplementation((_cmd, args: string[]) => (isIgnoredRgPass(args) ? p2 : p1)) + spawnMock.mockReturnValue(p1) const promise = listQuickOpenFiles('/mock/root', {} as unknown as Store) setTimeout(() => p1.emit('close', 127, null), 0) await expect(promise).rejects.toThrow(BUNDLED_ERROR) - expect(p2.kill).toHaveBeenCalled() }) it('rejects rg failures instead of resolving a false-empty list', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('/mock/root', storeMock) setTimeout(() => { p1.emit('close', 2, null) - p2.emit('close', 0, null) }, 10) await expect(promise).rejects.toThrow('rg exited with code 2') }) - it('kills the sibling rg pass after one pass fails', async () => { + it('does not start another scan after the admitted pass fails', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('/mock/root', storeMock) @@ -308,19 +298,13 @@ describe('filesystem-list-files', () => { }, 10) await expect(promise).rejects.toThrow('rg exited with code 2') - expect(p2.kill).toHaveBeenCalled() + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('accepts rg code 2 when rg emitted parseable paths first', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('/mock/root', storeMock) @@ -328,7 +312,6 @@ describe('filesystem-list-files', () => { setTimeout(() => { ;(p1.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') p1.emit('close', 2, null) - p2.emit('close', 0, null) }, 10) await expect(promise).resolves.toEqual(['src/index.ts']) @@ -339,14 +322,8 @@ describe('filesystem-list-files', () => { try { const p1 = createMockProcess() - const p2 = createMockProcess() - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('/mock/root', storeMock) @@ -362,7 +339,6 @@ describe('filesystem-list-files', () => { await rejection expect(p1.kill).toHaveBeenCalled() - expect(p2.kill).toHaveBeenCalled() expect((p1.stdout as unknown as EventEmitter).listenerCount('data')).toBe(0) expect((p1.stderr as unknown as EventEmitter).listenerCount('data')).toBe(0) expect(p1.listenerCount('error')).toBe(0) @@ -374,8 +350,7 @@ describe('filesystem-list-files', () => { it('kills local rg scans when a paired listing is cancelled', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() - spawnMock.mockImplementation((_cmd, args: string[]) => (isIgnoredRgPass(args) ? p2 : p1)) + spawnMock.mockReturnValue(p1) const controller = new AbortController() const cancellation = new FileListingCancelledError('superseded') const promise = listQuickOpenFiles( @@ -390,19 +365,12 @@ describe('filesystem-list-files', () => { await expect(promise).rejects.toBe(cancellation) expect(p1.kill).toHaveBeenCalledOnce() - expect(p2.kill).toHaveBeenCalledOnce() }) it('filters out .next, .cache, .stably, .vscode, .idea', async () => { const p1 = createMockProcess() - const p2 = createMockProcess() - spawnMock.mockImplementation((_cmd, args: string[]) => { - if (isIgnoredRgPass(args)) { - return p2 - } - return p1 - }) + spawnMock.mockReturnValue(p1) const storeMock = {} as unknown as Store const promise = listQuickOpenFiles('/mock/root', storeMock) @@ -415,9 +383,6 @@ describe('filesystem-list-files', () => { ;(p1.stdout as unknown as EventEmitter).emit('data', '.idea/workspace.xml\n') ;(p1.stdout as unknown as EventEmitter).emit('data', 'valid.ts\n') p1.emit('close', 0, null) - - // Empty ignored result - p2.emit('close', 0, null) }, 10) const result = await promise @@ -451,7 +416,7 @@ describe('filesystem-list-files', () => { }) describe('when the bundled rg cannot start', () => { - it('kills only the admitted pass when ignored rg fails before spawn', async () => { + it('does not kill a process that failed before receiving a pid', async () => { const primary = createMockProcess() const missingIgnored = createMockProcess() Object.defineProperty(missingIgnored, 'pid', { value: undefined }) @@ -461,11 +426,11 @@ describe('filesystem-list-files', () => { const promise = listQuickOpenFiles('/mock/root', {} as unknown as Store) await flushMicrotasks() - expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(1) missingIgnored.emit('close', -2, null) await expect(promise).rejects.toThrow(BUNDLED_ERROR) - expect(primary.kill).toHaveBeenCalled() + expect(primary.kill).not.toHaveBeenCalled() expect(missingIgnored.kill).not.toHaveBeenCalled() const error = Object.assign(new Error('spawn rg ENOENT'), { code: 'ENOENT' }) expect(() => missingIgnored.emit('error', error)).not.toThrow() diff --git a/src/main/ipc/filesystem-list-files.ts b/src/main/ipc/filesystem-list-files.ts index f1e8fb56334..4e3ea49dbe8 100644 --- a/src/main/ipc/filesystem-list-files.ts +++ b/src/main/ipc/filesystem-list-files.ts @@ -277,9 +277,7 @@ export async function listQuickOpenFiles( } const killSurvivors = (): void => { - // Why: if one rg pass fails, Promise.all rejects immediately while the - // sibling scan can keep walking a huge tree until timeout. Stop it so - // repeated Quick Open attempts do not accumulate local rg processes. + // Failed listings must release any process still walking the tree. for (const entry of children) { if (entry.isDone()) { continue @@ -303,16 +301,13 @@ export async function listQuickOpenFiles( } } try { - const primaryRun = runRg(primary) if (maxResults === undefined && maxSerializedBytes === undefined) { - // Why: a pid-less primary proves launch failure; avoid doubling the failed spawn. - await (children[0]?.child.pid === undefined - ? primaryRun - : Promise.all([primaryRun, runRg(ignoredPass)])) + // The broader pass already includes source files; an unbounded listing needs only one scan. + await runRg(ignoredPass) } else { // Why: ignored-file output can be much larger and faster than the primary pass; let source // files claim every bounded autocomplete budget first, including the transport byte cap. - await primaryRun + await runRg(primary) if ( (maxResults === undefined || files.size < maxResults) && (maxSerializedBytes === undefined || serializedBytes < maxSerializedBytes) diff --git a/src/relay/fs-handler-list-files-cancel.test.ts b/src/relay/fs-handler-list-files-cancel.test.ts index baf6101600c..97bea1b0813 100644 --- a/src/relay/fs-handler-list-files-cancel.test.ts +++ b/src/relay/fs-handler-list-files-cancel.test.ts @@ -44,26 +44,22 @@ describe('relay list-files cancellation', () => { vi.useRealTimers() }) - it('listFilesWithRg kills both rg passes and rejects when aborted mid-flight', async () => { - const primaryProc = createMockProcess() + it('listFilesWithRg kills the broad rg pass and rejects when aborted mid-flight', async () => { const ignoredProc = createMockProcess() - spawnMock.mockImplementation((_cmd: string, args: string[]) => - args.includes('--no-ignore-vcs') ? ignoredProc : primaryProc - ) + spawnMock.mockReturnValue(ignoredProc) const controller = new AbortController() const promise = listFilesWithRg('/remote/root', [], { signal: controller.signal }) // Partial output before the abort — must be discarded, not resolved. - ;(primaryProc.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') + ignoredProc.stdout?.emit('data', 'src/index.ts\n') controller.abort() await expect(promise).rejects.toSatisfy(isFileListingCancellation) - expect(primaryProc.kill).toHaveBeenCalled() + expect(spawnMock).toHaveBeenCalledTimes(1) expect(ignoredProc.kill).toHaveBeenCalled() // Late close events after cancellation must not fire anything. - primaryProc.emit('close', null, 'SIGTERM') ignoredProc.emit('close', null, 'SIGTERM') }) @@ -78,18 +74,14 @@ describe('relay list-files cancellation', () => { }) it('listFilesWithRg still resolves normally when a signal is provided but never aborted', async () => { - const primaryProc = createMockProcess() const ignoredProc = createMockProcess() - spawnMock.mockImplementation((_cmd: string, args: string[]) => - args.includes('--no-ignore-vcs') ? ignoredProc : primaryProc - ) + spawnMock.mockReturnValue(ignoredProc) const controller = new AbortController() const promise = listFilesWithRg('/remote/root', [], { signal: controller.signal }) setTimeout(() => { - ;(primaryProc.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') - primaryProc.emit('close', 0, null) + ignoredProc.stdout?.emit('data', 'src/index.ts\n') ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'dist/out.js\n') ignoredProc.emit('close', 0, null) }, 5) diff --git a/src/relay/fs-handler-list-files-ignored.test.ts b/src/relay/fs-handler-list-files-ignored.test.ts index 483afec1c4c..3f8b8c2b883 100644 --- a/src/relay/fs-handler-list-files-ignored.test.ts +++ b/src/relay/fs-handler-list-files-ignored.test.ts @@ -71,24 +71,16 @@ describe('relay quick open ignored file listing', () => { await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))) }) - it('rg ignored pass includes ignored non-env files and keeps blocklists/excludes', async () => { - const primaryProc = createMockProcess() + it('uses one broad rg pass for unbounded listings and keeps blocklists/excludes', async () => { const ignoredProc = createMockProcess() - spawnMock.mockImplementation((_cmd: string, args: string[]) => { - if (args.includes('--no-ignore-vcs')) { - return ignoredProc - } - return primaryProc - }) + spawnMock.mockReturnValue(ignoredProc) const promise = listFilesWithRg('/remote/root', ['packages/other']) - expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(1) setTimeout(() => { - ;(primaryProc.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') - primaryProc.emit('close', 0, null) - + ignoredProc.stdout?.emit('data', 'src/index.ts\n') ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'dist/generated.js\n') ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'node_modules/pkg/index.js\n') ;(ignoredProc.stdout as unknown as EventEmitter).emit('data', 'packages/other/src/x.ts\n') @@ -97,9 +89,7 @@ describe('relay quick open ignored file listing', () => { await expect(promise).resolves.toEqual(['src/index.ts', 'dist/generated.js']) - const ignoredArgs = spawnMock.mock.calls.find((call) => - (call[1] as string[]).includes('--no-ignore-vcs') - )?.[1] as string[] + const ignoredArgs = spawnMock.mock.calls[0][1] expect(ignoredArgs).toBeDefined() expect(ignoredArgs).toContain('--no-ignore-vcs') expect(ignoredArgs).not.toContain('.env*') @@ -150,6 +140,24 @@ describe('relay quick open ignored file listing', () => { await expect(promise).resolves.toEqual(['scripts/check-target.ts', 'src/components/target.ts']) }) + it('fills a bounded listing from primary files before admitting ignored files', async () => { + const primary = createMockProcess() + const broad = createMockProcess() + spawnMock.mockReturnValueOnce(primary).mockReturnValueOnce(broad) + + const promise = listFilesWithRg('/remote/root', [], { maxResults: 2 }) + expect(spawnMock).toHaveBeenCalledTimes(1) + expect(spawnMock.mock.calls[0][1]).not.toContain('--no-ignore-vcs') + primary.stdout?.emit('data', 'src/index.ts\n') + primary.emit('close', 0, null) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)) + expect(spawnMock.mock.calls[1][1]).toContain('--no-ignore-vcs') + broad.stdout?.emit('data', 'src/index.ts\ndist/generated.js\ndist/extra.js\n') + + await expect(promise).resolves.toEqual(['src/index.ts', 'dist/generated.js']) + expect(broad.kill).toHaveBeenCalled() + }) + it('retries a transient remote rg spawn failure without reporting ripgrep as missing', async () => { const failed = createMockProcess() const succeeded = createMockProcess() @@ -191,32 +199,27 @@ describe('relay quick open ignored file listing', () => { await expect(promise).resolves.toEqual(['src/target.ts']) }) - it('runs the ignored pass after an unbounded primary listing retry succeeds', async () => { - const failedPrimary = createMockProcess() - const succeededPrimary = createMockProcess() - const ignored = createMockProcess() - Object.defineProperty(failedPrimary, 'pid', { value: undefined }) - spawnMock - .mockReturnValueOnce(failedPrimary) - .mockReturnValueOnce(succeededPrimary) - .mockReturnValueOnce(ignored) + it('retries an unbounded broad listing once after a transient spawn failure', async () => { + const failed = createMockProcess() + const succeeded = createMockProcess() + Object.defineProperty(failed, 'pid', { value: undefined }) + spawnMock.mockReturnValueOnce(failed).mockReturnValueOnce(succeeded) const promise = listFilesWithRg('/remote/root') - failedPrimary.emit('error', Object.assign(new Error('spawn rg EAGAIN'), { code: 'EAGAIN' })) + failed.emit('error', Object.assign(new Error('spawn rg EAGAIN'), { code: 'EAGAIN' })) await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)) - ;(succeededPrimary.stdout as unknown as EventEmitter).emit('data', 'src/index.ts\n') - succeededPrimary.emit('close', 0, null) - await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(3)) - ;(ignored.stdout as unknown as EventEmitter).emit('data', 'dist/generated.js\n') - ignored.emit('close', 0, null) + succeeded.stdout?.emit('data', 'src/index.ts\ndist/generated.js\n') + succeeded.emit('close', 0, null) await expect(promise).resolves.toEqual(['src/index.ts', 'dist/generated.js']) - expect(spawnMock.mock.calls[2][1]).toContain('--no-ignore-vcs') + expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock.mock.calls[0][1]).toContain('--no-ignore-vcs') + expect(spawnMock.mock.calls[1][1]).toContain('--no-ignore-vcs') }) it.each(['error-first', 'close-first'] as const)( - 'tags a %s pre-spawn listing failure without starting the ignored pass', + 'tags a %s pre-spawn listing failure without starting another pass', async (order) => { const root = await makeTempRoot() const missing = createMockProcess() @@ -243,21 +246,17 @@ describe('relay quick open ignored file listing', () => { } ) - it('kills only the admitted pass when ignored rg fails before spawn', async () => { + it('does not signal the unbounded broad pass when it fails before spawn', async () => { const root = await makeTempRoot() - const primary = createMockProcess() const missingIgnored = createMockProcess() Object.defineProperty(missingIgnored, 'pid', { value: undefined }) - spawnMock.mockImplementation((_cmd: string, args: string[]) => - args.includes('--no-ignore-vcs') ? missingIgnored : primary - ) + spawnMock.mockReturnValue(missingIgnored) const promise = listFilesWithRg(root) - expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(1) missingIgnored.emit('close', -2, null) await expect(promise).rejects.toBeInstanceOf(RipgrepUnavailableError) - expect(primary.kill).toHaveBeenCalled() expect(missingIgnored.kill).not.toHaveBeenCalled() const error = Object.assign(new Error('spawn rg ENOENT'), { code: 'ENOENT' }) expect(() => missingIgnored.emit('error', error)).not.toThrow() @@ -548,14 +547,8 @@ describe('relay quick open ignored file listing', () => { it('rg file listing rejects and detaches when a timed-out child does not emit close', async () => { vi.useFakeTimers() try { - const primaryProc = createMockProcess() const ignoredProc = createMockProcess() - let callIndex = 0 - - spawnMock.mockImplementation(() => { - callIndex++ - return callIndex === 1 ? primaryProc : ignoredProc - }) + spawnMock.mockReturnValue(ignoredProc) const promise = listFilesWithRg('/remote/root') const outcomePromise = promise.then( @@ -567,12 +560,8 @@ describe('relay quick open ignored file listing', () => { const outcome = await Promise.race([outcomePromise, Promise.resolve('pending')]) expect(outcome).toBe('rejected:rg list timed out') - expect(primaryProc.kill).toHaveBeenCalled() + expect(spawnMock).toHaveBeenCalledTimes(1) expect(ignoredProc.kill).toHaveBeenCalled() - expect((primaryProc.stdout as unknown as EventEmitter).listenerCount('data')).toBe(0) - expect((primaryProc.stderr as unknown as EventEmitter).listenerCount('data')).toBe(0) - expect(primaryProc.listenerCount('error')).toBe(0) - expect(primaryProc.listenerCount('close')).toBe(0) expect((ignoredProc.stdout as unknown as EventEmitter).listenerCount('data')).toBe(0) expect((ignoredProc.stderr as unknown as EventEmitter).listenerCount('data')).toBe(0) expect(ignoredProc.listenerCount('error')).toBe(0) diff --git a/src/relay/fs-handler-list-files.ts b/src/relay/fs-handler-list-files.ts index d2b4732777c..506693fa0c8 100644 --- a/src/relay/fs-handler-list-files.ts +++ b/src/relay/fs-handler-list-files.ts @@ -6,7 +6,7 @@ * matching files" even though the file existed on disk. This implementation: * - streams via spawn (no maxBuffer failure mode) * - prunes traversal at rg level using the shared blocklist globs - * - runs a second --no-ignore-vcs pass for ignored files + * - includes gitignored files, preserving primary-first order for bounded listings * - honors excludePathPrefixes for nested linked worktrees * - rejects (not resolves) on timeout / spawn error / signal exit so * the UI shows a load error instead of a false-empty list @@ -284,10 +284,7 @@ export function listFilesWithRg( }) const killSurvivors = (reason: string): void => { - // Why: when one pass rejects, Promise.all surfaces the error immediately - // but the sibling rg keeps running up to LIST_FILES_TIMEOUT_MS. Kill it - // so repeated Quick Open opens don't pile up orphan rg processes on the - // remote. + // Cancellation or a reached budget must stop any admitted scan or retry. for (const entry of children) { if (entry.isDone()) { continue @@ -322,21 +319,13 @@ export function listFilesWithRg( } signal?.addEventListener('abort', onAbort, { once: true }) + // Without a result budget, the broader pass already contains every primary path. const passes = - searchQuery !== undefined + searchQuery !== undefined || maxResults === undefined ? runPass(ignoredPass) - : (() => { - const primaryPass = runPass(primary) - return maxResults === undefined - ? children[0]?.child.pid === undefined - ? primaryPass.then(() => runPass(ignoredPass)) - : Promise.all([primaryPass, runPass(ignoredPass)]) - : // Why: deterministic primary-first budgeting prevents a large ignored - // tree from starving ordinary source paths on a remote host. - primaryPass.then(() => - files.size < maxResults ? runPass(ignoredPass) : Promise.resolve() - ) - })() + : runPass(primary).then(() => + files.size < maxResults ? runPass(ignoredPass) : Promise.resolve() + ) passes .then(() => { @@ -353,7 +342,7 @@ export function listFilesWithRg( } done = true signal?.removeEventListener('abort', onAbort) - killSurvivors('rg list canceled after sibling failure') + killSurvivors('rg list canceled after failure') reject(err instanceof Error ? err : new Error(String(err))) }) }) diff --git a/src/shared/quick-open-filter.test.ts b/src/shared/quick-open-filter.test.ts index c481a1e8854..e142c8ef765 100644 --- a/src/shared/quick-open-filter.test.ts +++ b/src/shared/quick-open-filter.test.ts @@ -164,6 +164,19 @@ describe('buildHiddenDirExcludeGlobs', () => { }) describe('buildRgArgsForQuickOpen', () => { + it.each([ + { searchRoot: '.', excludePathPrefixes: [], forceSlashSeparator: false }, + { + searchRoot: '/root', + excludePathPrefixes: ['packages/app', 'feature[1]'], + forceSlashSeparator: true + } + ])('broadens only VCS ignore handling for $searchRoot', (options) => { + const { primary, ignoredPass } = buildRgArgsForQuickOpen(options) + expect(ignoredPass).toContain('--no-ignore-vcs') + expect(ignoredPass.filter((arg) => arg !== '--no-ignore-vcs')).toEqual(primary) + }) + it('primary pass includes --files, --hidden, hidden-dir excludes, no --follow', () => { const { primary } = buildRgArgsForQuickOpen({ searchRoot: '/root', diff --git a/src/shared/quick-open-filter.ts b/src/shared/quick-open-filter.ts index b7592657a11..ffb1ac346ef 100644 --- a/src/shared/quick-open-filter.ts +++ b/src/shared/quick-open-filter.ts @@ -183,7 +183,7 @@ export type RgArgsOptions = { export type RgArgs = { /** Main pass: all non-ignored files, hidden dotfiles included. */ primary: string[] - /** Second pass: ignored files, hidden dotfiles included. */ + /** Broader pass: primary files plus gitignored files, hidden dotfiles included. */ ignoredPass: string[] } From 4f50e4057852bba7f7953508cc76a6ae8e2a9935 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 3 Oct 2026 22:48:45 -0700 Subject: [PATCH 11/23] fix(mobile): never hand back a message the host recorded and then rejected The phone put a rejected message's text back in the composer even when the host had recorded it, so the same text showed in the chat as not sent and in the composer. The host's row now holds it, as on the desktop: the send reports that the host holds the text, so the composer and attachments are not restored. A message a Stop withdrew still goes back with its notice. --- mobile/src/session/mobile-native-chat-send.ts | 5 ++-- .../mobile-structured-send-delivery.test.ts | 6 ++-- .../mobile-structured-send-delivery.ts | 17 ++++++----- ...ile-structured-agent-session-send.test.tsx | 28 +++++++++++++++++-- ...bile-structured-native-chat-send-bridge.ts | 4 +-- 5 files changed, 44 insertions(+), 16 deletions(-) diff --git a/mobile/src/session/mobile-native-chat-send.ts b/mobile/src/session/mobile-native-chat-send.ts index 2a3bc5a60b2..22d2874b4ee 100644 --- a/mobile/src/session/mobile-native-chat-send.ts +++ b/mobile/src/session/mobile-native-chat-send.ts @@ -29,8 +29,9 @@ type MobileNativeChatSendArgs = { /** 'unknown' = the RPC failed without proof the request never reached the * desktop (ack loss after a write, or a cutover that cannot tell whether the * frame was written) — callers must not present it as a definite send failure. - * 'queued' = structured lane only: the host holds the message as a queued - * draft, so it shows as a card above the composer, never a transcript echo. */ + * 'queued' = structured lane only: the host holds the message, as a queued + * draft's card above the composer or as a recorded row in the transcript, so + * the client draws no echo and hands no text back. */ export type MobileNativeChatSendOutcome = 'accepted' | 'rejected' | 'unknown' | 'queued' /** What a terminal write can answer: the PTY lane has no draft queue. */ diff --git a/mobile/src/session/mobile-structured-send-delivery.test.ts b/mobile/src/session/mobile-structured-send-delivery.test.ts index bad17106e16..2178a8d2756 100644 --- a/mobile/src/session/mobile-structured-send-delivery.test.ts +++ b/mobile/src/session/mobile-structured-send-delivery.test.ts @@ -112,13 +112,13 @@ describe('mobileStructuredSendDelivery', () => { } }) - it('spends the id of a recorded rejection and leaves saying it to its row in the chat', () => { + it('spends the id of a recorded rejection and leaves the message to its row in the chat', () => { // Provably undelivered and terminal, so the id can only replay it: spending the // id makes the retry a first delivery. The host recorded the message, so the - // transcript shows it as not sent, with why; a banner would say it twice. + // transcript holds it and shows it as not sent, with why: no banner, no hand-back. for (const reason of ['provider_write_failed: broken pipe', 'Claude does not support .bmp']) { expect(mobileStructuredSendDelivery(accepted('rejected', reason))).toEqual({ - outcome: 'rejected', + outcome: 'queued', operationIdSpent: true, error: null }) diff --git a/mobile/src/session/mobile-structured-send-delivery.ts b/mobile/src/session/mobile-structured-send-delivery.ts index 55961aead50..a348b55a2c5 100644 --- a/mobile/src/session/mobile-structured-send-delivery.ts +++ b/mobile/src/session/mobile-structured-send-delivery.ts @@ -13,7 +13,8 @@ // // accepted/pending — the send happened. The id is spent; a later identical // message is a new message and must carry a new id. -// rejected — a terminal refusal or rejected submission spends a fresh id. A +// rejected — a terminal refusal or rejected submission spends a fresh id (a +// recorded one reports `queued`: the host holds its text, not the composer). 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 @@ -87,17 +88,19 @@ export function mobileStructuredSendDelivery( if (!submission || submission.dispatchState === 'unknown') { return { outcome: 'unknown', operationIdSpent: false, error: null } } - if (submission.dispatchState === 'rejected') { + if (submission.dispatchState === 'rejected' && dispatchWasWithdrawn(submission)) { + // A Stop withdrew it, so no row holds it: its text goes back to the composer, with why. return { outcome: 'rejected', operationIdSpent: true, - // The host recorded it, so its row in the chat says it was not sent and why; only a Stop's - // withdrawal leaves no row to say it. - error: dispatchWasWithdrawn(submission) - ? structuredAgentSessionRejectionNotice(submission.reason, 'composer-send') - : null + error: structuredAgentSessionRejectionNotice(submission.reason, 'composer-send') } } + if (submission.dispatchState === 'rejected') { + // The host recorded it: its row in the chat holds the text and says it was not sent and why, + // so nothing is handed back. + return { outcome: 'queued', operationIdSpent: true, error: null } + } if (retained) { // A payload match cannot distinguish retrying the ambiguous action from a // later identical intent. Wait for the stream to settle and release it. diff --git a/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx b/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx index 34b66f1dd1f..85640ff842e 100644 --- a/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx +++ b/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx @@ -21,13 +21,13 @@ function ok(result: unknown) { return { ok: true, result, _meta: { runtimeId: 'runtime-1' } } } -function sendResult(dispatchState: AgentJournalDispatchState) { +function sendResult(dispatchState: AgentJournalDispatchState, reason: string | null = null) { return ok({ ok: true, replayed: false, fence: 3, cursor: { epoch: 'epoch-1', sequence: 1 }, - value: structuredSendResultFixture(dispatchState) + value: structuredSendResultFixture(dispatchState, reason) }) } @@ -257,6 +257,30 @@ describe('mobile structured send retries', () => { expect(calls().every(([, params]) => !('retryUnknown' in (params as object)))).toBe(true) }) + // The transcript owns a message the host recorded, so the composer never gets it back. + it('hands back only a send a Stop withdrew, never one the host recorded and rejected', async () => { + let reason = 'provider_write_failed: broken pipe' + sendRequest.mockImplementation(async (method) => + method === 'agentSession.send' + ? sendResult('rejected', reason) + : method === 'agentSession.options' + ? ok({ models: [], current: {} }) + : ok({}) + ) + await mountSession() + + await act(async () => { + expect(await hook!.sendWithOutcome('recorded, then rejected')).toBe('queued') + }) + expect(onSendError).not.toHaveBeenCalled() + + reason = 'provider_cancelled_before_start' + await act(async () => { + expect(await hook!.sendWithOutcome('withdrawn by Stop')).toBe('rejected') + }) + expect(onSendError).toHaveBeenCalledWith('Your message was not sent. Send it again.') + }) + it('reuses the original uploaded attachment identity after acknowledgement loss', async () => { let attempts = 0 sendRequest.mockImplementation(async (method) => { diff --git a/mobile/src/session/use-mobile-structured-native-chat-send-bridge.ts b/mobile/src/session/use-mobile-structured-native-chat-send-bridge.ts index 5abe42b39e6..f8b9d7f64b2 100644 --- a/mobile/src/session/use-mobile-structured-native-chat-send-bridge.ts +++ b/mobile/src/session/use-mobile-structured-native-chat-send-bridge.ts @@ -77,8 +77,8 @@ export function useMobileStructuredNativeChatSendBridge(args: { return 'accepted' } if (outcome === 'queued') { - // The host holds the draft and publishes it as a card above the - // composer — never an optimistic transcript bubble. + // The host holds the text, as a card above the composer or a recorded + // row in the transcript — never an optimistic bubble or a hand-back. return 'queued' } if (outcome === 'unknown') { From e6c5c84546227d142817a22188101fed2b7e6369 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 3 Oct 2026 22:54:09 -0700 Subject: [PATCH 12/23] fix(native-chat): a not-sent line reads muted, not as an error The line under a message that was not sent, on desktop and phone, used the error color, as if the user had something to fix. It is a plain label now, in the muted text color. A line that is still in doubt ("Message delivery is unconfirmed.", or an expired id Orca can't confirm) keeps the error color, as does the terminal chat's notice. --- .../session/MobileNativeChatMessage.test.ts | 10 +++++ .../mobile-native-chat-message-styles.ts | 2 +- .../native-chat/NativeChatMessageRow.test.tsx | 16 ++++++- .../native-chat/NativeChatMessageRow.tsx | 9 +++- ...red-agent-session-delivery-notices.test.ts | 45 ++++++++++++++++++- ...ructured-agent-session-delivery-notices.ts | 29 ++++++++---- 6 files changed, 99 insertions(+), 12 deletions(-) diff --git a/mobile/src/session/MobileNativeChatMessage.test.ts b/mobile/src/session/MobileNativeChatMessage.test.ts index d96d26c3100..e92a096e18b 100644 --- a/mobile/src/session/MobileNativeChatMessage.test.ts +++ b/mobile/src/session/MobileNativeChatMessage.test.ts @@ -3,6 +3,7 @@ import { act, create, type ReactTestInstance, type ReactTestRenderer } from 'rea import { afterEach, describe, expect, it, vi } from 'vitest' import { MAX_TOOL_DETAIL_LENGTH } from '../../../src/shared/native-chat-tool-summary' import type { NativeChatMessage } from '../../../src/shared/native-chat-types' +import { colors } from '../theme/mobile-theme' vi.mock('react-native', async () => { const React = await import('react') @@ -124,6 +125,15 @@ describe('MobileNativeChatMessage', () => { expect(textIn(tree.root)).toEqual(['hello', 'Your message was not sent.']) }) + it('says it was not sent as a muted label, not an error', () => { + const tree = render({ ...userMessage([{ type: 'text', text: 'hello' }]), unsent: true }) + const [label] = tree.root.findAll( + (node) => + node.props.style !== undefined && node.children.join('') === 'Your message was not sent.' + ) + expect(label?.props.style).toMatchObject({ color: colors.textMuted }) + }) + it('says nothing more under a delivered message', () => { const tree = render(userMessage([{ type: 'text', text: 'hello' }])) expect(textIn(tree.root)).toEqual(['hello']) diff --git a/mobile/src/session/mobile-native-chat-message-styles.ts b/mobile/src/session/mobile-native-chat-message-styles.ts index b3455c89830..18895b648ec 100644 --- a/mobile/src/session/mobile-native-chat-message-styles.ts +++ b/mobile/src/session/mobile-native-chat-message-styles.ts @@ -34,7 +34,7 @@ export const styles = StyleSheet.create({ }, unsentLabel: { marginTop: spacing.xs, - color: colors.statusRed, + color: colors.textMuted, fontSize: typography.metaSize }, toolRun: { diff --git a/src/renderer/src/components/native-chat/NativeChatMessageRow.test.tsx b/src/renderer/src/components/native-chat/NativeChatMessageRow.test.tsx index bcecab05bc6..91a54f3c9f9 100644 --- a/src/renderer/src/components/native-chat/NativeChatMessageRow.test.tsx +++ b/src/renderer/src/components/native-chat/NativeChatMessageRow.test.tsx @@ -164,7 +164,7 @@ describe('MessageRow send mode', () => { }) describe('a user message that did not go through', () => { - function renderUser(deliveryNotice?: { text: string; onRetry?: () => void }) { + function renderUser(deliveryNotice?: { text: string; notSent?: true; onRetry?: () => void }) { return render( { expect(screen.queryByRole('button', { name: 'Retry' })).toBeNull() }) + // A plain "not sent" is a label, not an error; a doubt to check keeps the error color. + it('reads muted when it says only that the message was not sent', () => { + renderUser({ text: 'Your message was not sent.', notSent: true }) + const notSent = screen.getByText('Your message was not sent.').parentElement + expect(notSent).toHaveClass('text-muted-foreground') + expect(notSent).not.toHaveClass('text-destructive/80') + cleanup() + + renderUser({ text: 'Message delivery is unconfirmed.' }) + expect(screen.getByText('Message delivery is unconfirmed.').parentElement).toHaveClass( + 'text-destructive/80' + ) + }) + it('says nothing when it went through', () => { renderUser() expect(screen.queryByRole('button', { name: 'Retry' })).toBeNull() diff --git a/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx b/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx index 234934664f4..07542ea89c5 100644 --- a/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx +++ b/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx @@ -32,6 +32,8 @@ import type { RuntimeFileOperationArgs } from '@/runtime/runtime-file-client' * surface can send it again. */ export type NativeChatDeliveryNotice = { text: string + /** Says only that the message did not go out, so it reads muted, not as an error. */ + notSent?: true onRetry?: () => void onDismiss?: () => void } @@ -187,7 +189,12 @@ export const MessageRow = memo(function MessageRow({ ) : null} {deliveryNotice ? ( -
+
{deliveryNotice.text} {deliveryNotice.onDismiss ? ( + + + + + ) +})) + +const item: GitHubWorkItem = { + id: 'issue:5', + repoId: 'registered-repo', + type: 'issue', + number: 5, + title: 'Issue five', + state: 'open', + url: 'https://github.com/upstream/widgets/issues/5', + labels: [], + updatedAt: '', + author: null +} +let root: Root +let container: HTMLDivElement + +async function render( + nextItem = item, + assignees: string[] = [], + projectOrigin?: GitHubItemDialogProjectOrigin +): Promise { + await act(async () => { + root.render( + {}} + onLabelsChange={() => {}} + onMutated={mocks.onMutated} + assignees={assignees} + onUse={() => {}} + layout="top-columns" + /> + ) + }) +} + +async function click(label = 'Assign candidate'): Promise { + const button = [...container.querySelectorAll('button')].find( + (candidate) => candidate.textContent === label + ) + expect(button).toBeDefined() + await act(async () => button?.click()) +} + +function selected(): string | null { + return container.querySelector('[data-field="assignees"]')?.textContent ?? null +} + +let updateParentSelection: (nextItem: GitHubWorkItem) => void = () => {} + +function ParentSelection({ item }: { item: GitHubWorkItem }): React.JSX.Element { + const [localState, setLocalState] = useState(item.state) + const [localLabels, setLocalLabels] = useState(item.labels) + updateParentSelection = (nextItem) => { + setLocalState(nextItem.state) + setLocalLabels(nextItem.labels) + } + return ( + {}} + layout="top-columns" + /> + ) +} + +async function renderParent(nextItem: GitHubWorkItem): Promise { + await act(async () => { + updateParentSelection(nextItem) + root.render() + }) +} + +describe('opened issue assignee ownership', () => { + beforeEach(() => { + vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true) + vi.clearAllMocks() + resetTaskPageGitHubMutationRegistryForTests() + updateParentSelection = () => {} + mocks.update.mockResolvedValue({ ok: true }) + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + }) + + afterEach(() => { + act(() => root.unmount()) + container.remove() + vi.unstubAllGlobals() + resetTaskPageGitHubMutationRegistryForTests() + }) + + it('retires an optimistic selection across fork and return navigation', async () => { + await render() + await click() + expect(selected()).toBe('candidate-user') + await render(item, []) + expect(selected()).toBe('candidate-user') + + await render({ ...item, url: 'https://github.com/fork/widgets/issues/5' }, ['fork-user']) + expect(selected()).toBe('fork-user') + await render(item, ['authoritative-upstream-user']) + expect(selected()).toBe('authoritative-upstream-user') + }) + + it('does not revive an edited guard when returning from a different issue number', async () => { + await render() + await click() + await render({ ...item, id: 'issue:6', number: 6 }, ['other-issue-user']) + expect(selected()).toBe('other-issue-user') + await render(item, ['authoritative-upstream-user']) + expect(selected()).toBe('authoritative-upstream-user') + }) + + it.each(['Assign candidate', 'Toggle original'])( + 'does not roll a late failed %s edit into another opened issue', + async (button) => { + let rejectUpdate: (reason: Error) => void = () => {} + mocks.update.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectUpdate = reject + }) + ) + await render(item, ['original-user']) + await click(button) + await render({ ...item, id: 'issue:6', number: 6 }, ['other-issue-user']) + expect(selected()).toBe('other-issue-user') + await act(async () => rejectUpdate(new Error('held edit failed'))) + expect(selected()).toBe('other-issue-user') + } + ) + + it('treats another GitHub host as a distinct same-number issue', async () => { + await render() + await click() + await render({ ...item, url: 'https://github.example.test/upstream/widgets/issues/5' }, []) + expect(selected()).toBe('') + }) + + it('retires the issue edit when the same legacy id is used by a pull request', async () => { + await render() + await click() + await render({ ...item, type: 'pr' }, []) + await render(item, ['authoritative-upstream-user']) + expect(selected()).toBe('authoritative-upstream-user') + }) + + it('preserves optimistic edits across canonical URL casing and trailing paths', async () => { + await render() + await click() + await render({ ...item, url: 'https://GITHUB.COM/Upstream/Widgets/issues/5#activity' }, []) + expect(selected()).toBe('candidate-user') + }) + + it('uses the Project row repository before an unrelated item URL', async () => { + const projectOrigin: GitHubItemDialogProjectOrigin = { + owner: 'upstream', + repo: 'widgets', + number: 5, + type: 'issue', + projectId: 'project', + projectItemId: 'row', + cacheKey: 'project-cache' + } + await render(item, [], projectOrigin) + await click() + await render( + { ...item, url: 'https://github.com/unrelated/widgets/issues/5' }, + [], + projectOrigin + ) + expect(selected()).toBe('candidate-user') + await render(item, ['fork-user'], { ...projectOrigin, owner: 'fork' }) + expect(selected()).toBe('fork-user') + }) + + it('still rolls back a failed edit while its issue remains open', async () => { + mocks.update.mockRejectedValue(new Error('edit failed')) + await render(item, ['original-user']) + await click() + expect(selected()).toBe('original-user') + await render(item, ['refetched-user']) + expect(selected()).toBe('refetched-user') + }) + + it('keeps a late Project-row rollback scoped to the captured row', async () => { + const projectOrigin: GitHubItemDialogProjectOrigin = { + owner: 'upstream', + repo: 'widgets', + number: 5, + type: 'issue', + projectId: 'project', + projectItemId: 'upstream-row', + cacheKey: 'upstream-cache' + } + let rejectUpdate: (reason: Error) => void = () => {} + mocks.update.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectUpdate = reject + }) + ) + await render(item, ['original-user'], projectOrigin) + await click() + await render(item, ['fork-user'], { + ...projectOrigin, + owner: 'fork', + projectItemId: 'fork-row', + cacheKey: 'fork-cache' + }) + expect(selected()).toBe('fork-user') + await act(async () => rejectUpdate(new Error('held Project edit failed'))) + expect(selected()).toBe('fork-user') + expect(mocks.patchProjectRowContent).toHaveBeenLastCalledWith( + 'upstream-cache', + 'upstream-row', + { assignees: ['original-user'] } + ) + }) + + it.each([ + { button: 'Toggle label', field: 'labels', expected: 'fork-only-label' }, + { button: 'Close issue', field: 'state', expected: 'closed' } + ])( + 'a late failed $field edit cannot overwrite the new parent selection', + async ({ button, field, expected }) => { + let rejectUpdate: (reason: Error) => void = () => {} + mocks.update.mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectUpdate = reject + }) + ) + await renderParent({ ...item, labels: ['upstream-original-label'] }) + await click(button) + await renderParent({ + ...item, + state: 'closed', + url: 'https://github.com/fork/widgets/issues/5', + labels: ['fork-only-label'] + }) + expect(container.querySelector(`[data-field="${field}"]`)?.textContent).toBe(expected) + await act(async () => rejectUpdate(new Error('held edit failed'))) + expect(container.querySelector(`[data-field="${field}"]`)?.textContent).toBe(expected) + expect(mocks.patchWorkItem).toHaveBeenLastCalledWith( + item.id, + field === 'labels' ? { labels: ['upstream-original-label'] } : { state: 'open' }, + item.repoId, + { + sourceContext: undefined, + ownerRepo: { owner: 'upstream', repo: 'widgets', host: 'github.com' } + } + ) + } + ) + + it.each([ + { button: 'Toggle label', field: 'labels', expected: 'upstream-original-label' }, + { button: 'Close issue', field: 'state', expected: 'open' } + ])( + 'a same-item failed $field edit still restores its prior parent value', + async ({ button, field, expected }) => { + mocks.update.mockRejectedValue(new Error('same issue edit failed')) + await renderParent({ ...item, labels: ['upstream-original-label'] }) + await click(button) + expect(container.querySelector(`[data-field="${field}"]`)?.textContent).toBe(expected) + } + ) +}) diff --git a/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx index f92c99b2a09..a50f41fc960 100644 --- a/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx +++ b/src/renderer/src/components/github-item-dialog/edit-item-fields/gh-edit-section.tsx @@ -3,11 +3,8 @@ import { useShallow } from 'zustand/react/shallow' import { useAppStore } from '@/store' import { useRepoLabels, useRepoAssignees, useImmediateMutation } from '@/hooks/useIssueMetadata' import { useRepoLabelsBySlug, useRepoAssigneesBySlug } from '@/hooks/useGitHubSlugMetadata' -import { - getTaskSourceRuntimeSettings, - type TaskSourceContext -} from '../../../../../shared/task-source-context' -import type { GitHubWorkItem } from '../../../../../shared/github/work-item-types' +import { getTaskSourceRuntimeSettings } from '../../../../../shared/task-source-context' +import { githubRepoIdentityKey } from '../../../../../shared/github/repository-identity-key' import { getSettingsForRepoRuntimeOwner } from '@/lib/repo-runtime-owner' import { getTaskPageGitHubDuplicateCandidates, @@ -17,7 +14,8 @@ import { } from '@/components/task-page-github-status-actions' import { parseOwnerRepoFromItemUrl } from '@/components/github/github-work-item-identity' import { translate } from '@/i18n/i18n' -import type { GitHubItemDialogProjectOrigin } from '../load-item-details/github-item-dialog-types' +import type { GitHubItemDialogEditSectionProps } from '../load-item-details/github-item-dialog-types' +import { useMountedRef } from '@/hooks/useMountedRef' import { getGitHubRepositoryLabelsUrl } from './repository-labels-url' import { closeGHEditAsDuplicate, @@ -29,7 +27,14 @@ import { GHEditSectionTopColumns } from './gh-edit-section-top-columns' import { GHEditSectionHorizontal } from './gh-edit-section-horizontal' import { useGitHubDuplicateIssueCandidates } from '@/components/github/github-duplicate-issue-candidates' -export function GHEditSection({ +export function GHEditSection(props: GitHubItemDialogEditSectionProps): React.JSX.Element | null { + const { item, projectOrigin } = props + const repository = projectOrigin ?? parseOwnerRepoFromItemUrl(item.url) + const itemKey = `${item.repoId}\0${repository ? githubRepoIdentityKey(repository) : item.url}\0${item.type}\0${item.id}` + return +} + +function GHEditSectionItem({ item, repoPath, repoId, @@ -45,25 +50,7 @@ export function GHEditSection({ onOpenOrUse, attachedWorkspaceLabel, layout = 'horizontal' -}: { - item: GitHubWorkItem - repoPath: string | null - repoId: string | null - sourceContext?: TaskSourceContext | null - projectOrigin: GitHubItemDialogProjectOrigin | undefined - localState: GitHubWorkItem['state'] - localLabels: string[] - onStateChange: (state: GitHubWorkItem['state']) => void - onLabelsChange: (labels: string[]) => void - /** Why: lets the parent invalidate its details cache after a mutation, else a reopen within FRESH_MS paints pre-mutation data. */ - onMutated: () => void - assignees: string[] - onUse: (item: GitHubWorkItem) => void - onOpenOrUse?: (item: GitHubWorkItem) => void - attachedWorkspaceLabel?: string | null - /** `horizontal`: compact pill strip for the non-issue drawer/header; `top-columns`: labeled columns above the issue page body. */ - layout?: 'horizontal' | 'top-columns' -}): React.JSX.Element | null { +}: GitHubItemDialogEditSectionProps): React.JSX.Element | null { const [labelPopoverOpen, setLabelPopoverOpen] = useState(false) const [assigneePopoverOpen, setAssigneePopoverOpen] = useState(false) const [statusPopoverOpen, setStatusPopoverOpen] = useState(false) @@ -72,6 +59,7 @@ export function GHEditSection({ const [duplicateError, setDuplicateError] = useState(null) const [localAssignees, setLocalAssignees] = useState(assignees) const editedAssigneesItemKeyRef = useRef(null) + const mountedRef = useMountedRef() const assigneesItemKey = `${item.repoId}\0${item.id}` const patchWorkItem = useAppStore((s) => s.patchWorkItem) const patchProjectRowContent = useAppStore((s) => s.patchProjectRowContent) @@ -101,13 +89,18 @@ export function GHEditSection({ [projectOrigin, patchProjectRowContent] ) - // Why: with projectOrigin set, read labels/assignees from the row's repo, not the workspace path, or popovers list a different repo than writes target. + const issueRepo = useMemo(() => parseOwnerRepoFromItemUrl(item.url), [item.url]) + const metadataOptions = useMemo( + () => ({ ...sourceSettings, ownerRepo: issueRepo }), + [sourceSettings, issueRepo] + ) + // Project metadata comes from the row repository. const slugOwner = projectOrigin?.owner ?? null const slugRepo = projectOrigin?.repo ?? null const repoLabelsByPath = useRepoLabels( projectOrigin ? null : repoPath, projectOrigin ? null : repoId, - sourceSettings + metadataOptions ) const repoLabelsBySlug = useRepoLabelsBySlug( slugOwner, @@ -120,7 +113,7 @@ export function GHEditSection({ const repoAssigneesByPath = useRepoAssignees( projectOrigin ? null : repoPath, projectOrigin ? null : repoId, - sourceSettings + metadataOptions ) const repoAssigneesBySlug = useRepoAssigneesBySlug( slugOwner, @@ -187,8 +180,13 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, run, - onStateChange, + onStateChange: (state) => { + if (mountedRef.current) { + onStateChange(state) + } + }, patchWorkItem, patchProjectRowIfNeeded, onMutated @@ -202,9 +200,11 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, patchWorkItem, patchProjectRowIfNeeded, run, + mountedRef, onStateChange, onMutated ] @@ -253,8 +253,13 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, run, - onLabelsChange, + onLabelsChange: (labels) => { + if (mountedRef.current) { + onLabelsChange(labels) + } + }, patchWorkItem, patchProjectRowIfNeeded, onMutated @@ -268,9 +273,11 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, patchWorkItem, patchProjectRowIfNeeded, run, + mountedRef, onLabelsChange, onMutated ] @@ -288,6 +295,7 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, run, setLocalAssignees, patchProjectRowIfNeeded, @@ -301,6 +309,7 @@ export function GHEditSection({ repoPath, sourceContext, projectOrigin, + issueRepo, localAssignees, patchProjectRowIfNeeded, run, diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts b/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts index c0a6fc7a3b8..9c8e81953b2 100644 --- a/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts +++ b/src/renderer/src/components/github-item-dialog/load-item-details/github-item-dialog-types.ts @@ -26,3 +26,23 @@ export type GitHubItemDialogProps = { /** Optional Project-origin context; when set, edits route via slug-addressed IPCs against the row's repo (slug routing wins for writes). */ projectOrigin?: GitHubItemDialogProjectOrigin } + +export type GitHubItemDialogEditSectionProps = { + item: GitHubWorkItem + repoPath: string | null + repoId: string | null + sourceContext?: TaskSourceContext | null + projectOrigin: GitHubItemDialogProjectOrigin | undefined + localState: GitHubWorkItem['state'] + localLabels: string[] + onStateChange: (state: GitHubWorkItem['state']) => void + onLabelsChange: (labels: string[]) => void + /** Why: lets the parent invalidate its details cache after a mutation, else a reopen within FRESH_MS paints pre-mutation data. */ + onMutated: () => void + assignees: string[] + onUse: (item: GitHubWorkItem) => void + onOpenOrUse?: (item: GitHubWorkItem) => void + attachedWorkspaceLabel?: string | null + /** `horizontal`: compact pill strip for the non-issue drawer/header; `top-columns`: labeled columns above the issue page body. */ + layout?: 'horizontal' | 'top-columns' +} diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts b/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts index 0d685da6020..c9bd69d9bb1 100644 --- a/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts +++ b/src/renderer/src/components/github-item-dialog/load-item-details/use-github-item-dialog-details.ts @@ -4,6 +4,7 @@ import { lookupGitHubWorkItemDetailsForSource } from '@/lib/github-work-item-sou import { canUseGitHubRepoContext } from '@/lib/github-source-runtime-context' import { normalizeItemDialogTab, + parseOwnerRepoFromItemUrl, type ItemDialogTab } from '@/components/github/github-work-item-identity' import type { PRComment } from '../../../../../shared/github/comment-types' @@ -61,6 +62,13 @@ export function useGitHubItemDialogDetails({ ?.issueSourcePreference }) const canUseDetailsRepoContext = canUseGitHubRepoContext(repoPath, sourceContext) + const issueRepository = useMemo( + () => + workItem?.type === 'issue' + ? (projectOrigin ?? parseOwnerRepoFromItemUrl(workItem.url)) + : null, + [projectOrigin, workItem] + ) const detailsCacheKey = useMemo(() => { if (!workItem || !effectiveRepoId || !canUseDetailsRepoContext) { return null @@ -69,10 +77,12 @@ export function useGitHubItemDialogDetails({ repoPath: repoPath ?? '', repoId: effectiveRepoId, issueSourcePreference, + sourceContext, sourceCacheScope: sourceContext?.provider === 'github' ? getTaskSourceCacheScope(sourceContext) : null, type: workItem.type, - number: workItem.number + number: workItem.number, + ownerRepo: issueRepository }) }, [ canUseDetailsRepoContext, @@ -80,7 +90,8 @@ export function useGitHubItemDialogDetails({ effectiveRepoId, sourceContext, workItem, - issueSourcePreference + issueSourcePreference, + issueRepository ]) // Why: reset during render so an item switch never paints the previous item's tab. @@ -97,7 +108,7 @@ export function useGitHubItemDialogDetails({ } // Why: hold comments added before the detail fetch resolves so they merge into the result instead of being overwritten. - const optimisticCommentsRef = useRef([]) + const optimisticCommentsRef = useRef(new Map()) // Why: distinguish "reopen same item" from "switch item" — reopen must keep optimistic comments since gh's 60s cache omits the just-posted one. const prevItemIdRef = useRef(null) @@ -116,7 +127,7 @@ export function useGitHubItemDialogDetails({ // Why: key off cachedEntry identity (stable), not the optimistic ref array (fresh each render), to avoid needless recompute. const details = useMemo(() => { const cachedDetails = cachedEntry?.details ?? null - const opt = optimisticCommentsRef.current + const opt = optimisticCommentsRef.current.get(detailsCacheKey ?? '') ?? [] if (!cachedDetails) { // Why: on cold open, details may still be loading — surface optimistic comments via a minimal shell so a pre-fetch comment isn't invisible. if (opt.length > 0 && workItem) { @@ -138,7 +149,7 @@ export function useGitHubItemDialogDetails({ } // Why: optimisticTick forces this ref-reading memo to re-run on cold-open writes; lint can't see the dependency. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [cachedEntry, workItem, optimisticTick]) + }, [cachedEntry, workItem, detailsCacheKey, optimisticTick]) const loading = !!cachedEntry?.pending && !cachedEntry?.details const error = cachedEntry?.error && !cachedEntry?.details ? cachedEntry.error : null @@ -158,7 +169,7 @@ export function useGitHubItemDialogDetails({ } // Why: clear optimistic comments only on item switch — on reopen, gh's 60s cache omits the just-posted comment, so keep the ref to re-merge. if (workItem.id !== prevItemIdRef.current) { - optimisticCommentsRef.current = [] + optimisticCommentsRef.current.clear() } prevItemIdRef.current = workItem.id @@ -178,7 +189,8 @@ export function useGitHubItemDialogDetails({ repoId: effectiveRepoId, sourceContext, number: workItem.number, - type: workItem.type + type: workItem.type, + ownerRepo: issueRepository }) // Why: snapshot the invalidation generation; if it advances before resolve, a mid-flight mutation invalidated the entry — don't write back. @@ -201,6 +213,7 @@ export function useGitHubItemDialogDetails({ sourceContext, workItem, detailsCacheKey, + issueRepository, refetchTick ]) @@ -231,7 +244,9 @@ export function useGitHubItemDialogDetails({ (comment: PRComment) => { useAppStore.getState().recordFeatureInteraction('github-tasks') // Why: skip refreshDetails() — gh's 60s cache would overwrite the optimistic comment; next open picks up the server version. - optimisticCommentsRef.current.push(comment) + const optimisticKey = detailsCacheKey ?? '' + const optimisticComments = optimisticCommentsRef.current.get(optimisticKey) ?? [] + optimisticCommentsRef.current.set(optimisticKey, [...optimisticComments, comment]) // Why: write through the module cache so concurrent drawers re-render; mark fetchedAt stale (0) so next open refetches server fields. if (detailsCacheKey) { const prev = workItemDetailsCache.get(detailsCacheKey) diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts index 36087ef00b2..38811153a28 100644 --- a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts +++ b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-cache.ts @@ -1,9 +1,13 @@ import type { PRCheckDetail } from '../../../../../shared/github/check-types' import type { GitHubAssignableUser, + GitHubOwnerRepo, GitHubPRFileViewedState } from '../../../../../shared/github/pull-request-types' +import { githubRepoIdentityKey } from '../../../../../shared/github/repository-identity-key' import type { GitHubWorkItemDetails } from '../../../../../shared/github/work-item-types' +import type { TaskSourceContext } from '../../../../../shared/task-source-context' +import { getGitHubSourceRuntimeHost } from '@/lib/github-source-runtime-context' import { onGitHubWorkItemDetailsCacheMutation } from '@/lib/github-work-item-details-cache-events' // Why: SWR cache for work-item details so reopening paints instantly instead of paying IPC + `gh` startup; keyed to avoid source/type collisions, LRU-bounded, FRESH_MS refetch on open. See docs/gh-work-item-drawer-cache.md. @@ -37,20 +41,20 @@ export function getWorkItemDetailsCacheKey(args: { repoId: string issueSourcePreference: string | undefined sourceCacheScope?: string | null + sourceContext?: TaskSourceContext | null type: 'issue' | 'pr' number: number + ownerRepo?: GitHubOwnerRepo | null }): string { // Why: key on every axis that changes which (repo, item) the IPC resolves to; `\0` separator avoids ambiguity with fields containing `:` or `/`. // Why: repoPath is the second part so match-based invalidation can find entries from a cross-window event that carries only the path. + const sourceKey = + args.ownerRepo && !getGitHubSourceRuntimeHost(args.sourceContext) + ? githubRepoIdentityKey(args.ownerRepo) + : (args.issueSourcePreference ?? 'auto') const keyParts = args.sourceCacheScope - ? [ - args.repoId, - args.repoPath, - args.sourceCacheScope, - args.issueSourcePreference ?? 'auto', - args.type - ] - : [args.repoId, args.repoPath, args.issueSourcePreference ?? 'auto', args.type] + ? [args.repoId, args.repoPath, args.sourceCacheScope, sourceKey, args.type] + : [args.repoId, args.repoPath, sourceKey, args.type] return [...keyParts, args.number].join('\0') } diff --git a/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-issue-target.test.ts b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-issue-target.test.ts new file mode 100644 index 00000000000..a5c7ab456c8 --- /dev/null +++ b/src/renderer/src/components/github-item-dialog/load-item-details/work-item-details-issue-target.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it, vi } from 'vitest' +import type { TaskSourceContext } from '../../../../../shared/task-source-context' +import { + getWorkItemDetailsCacheKey, + invalidateWorkItemDetailsCacheByMatch, + touchWorkItemDetailsCache, + workItemDetailsCache +} from './work-item-details-cache' + +vi.mock('@/lib/github-work-item-details-cache-events', () => ({ + onGitHubWorkItemDetailsCacheMutation: vi.fn() +})) + +const keyArgs = { + repoId: 'repo-1', + repoPath: '/home/fixture/widgets', + type: 'issue' as const, + number: 12, + issueSourcePreference: 'origin' +} +const ORIGIN = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } +const UPSTREAM = { owner: 'upstream-owner', repo: 'widgets', host: 'github.com' } +const LOCAL_SOURCE: TaskSourceContext = { + kind: 'task-source', + provider: 'github', + projectId: 'project-1', + hostId: 'local', + repoId: 'repo-1' +} + +describe('issue detail cache repository identity', () => { + it('separates equal issue numbers across origin and upstream', () => { + expect(getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: ORIGIN })).not.toBe( + getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: UPSTREAM }) + ) + }) + + it.each([undefined, LOCAL_SOURCE])( + 'keeps a local opened issue key stable when another window changes the selector: %j', + (sourceContext) => { + expect(getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: ORIGIN, sourceContext })).toBe( + getWorkItemDetailsCacheKey({ + ...keyArgs, + issueSourcePreference: 'upstream', + ownerRepo: ORIGIN, + sourceContext + }) + ) + } + ) + + it('keeps RPC detail caches scoped to the preference used by their existing lookup', () => { + const sourceContext: TaskSourceContext = { + kind: 'task-source', + provider: 'github', + projectId: 'project-1', + hostId: 'runtime:env-1', + repoId: 'runtime-repo' + } + const args = { ...keyArgs, ownerRepo: ORIGIN, sourceContext } + + expect(getWorkItemDetailsCacheKey(args)).not.toBe( + getWorkItemDetailsCacheKey({ ...args, issueSourcePreference: 'upstream' }) + ) + expect(getWorkItemDetailsCacheKey(args)).toBe( + getWorkItemDetailsCacheKey({ ...args, ownerRepo: null }) + ) + }) + + it('invalidates both repository variants after a mutation', () => { + const originKey = getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: ORIGIN }) + const upstreamKey = getWorkItemDetailsCacheKey({ ...keyArgs, ownerRepo: UPSTREAM }) + touchWorkItemDetailsCache(originKey, { details: null, fetchedAt: 0 }) + touchWorkItemDetailsCache(upstreamKey, { details: null, fetchedAt: 0 }) + + invalidateWorkItemDetailsCacheByMatch(keyArgs) + + expect(workItemDetailsCache.has(originKey)).toBe(false) + expect(workItemDetailsCache.has(upstreamKey)).toBe(false) + }) +}) diff --git a/src/renderer/src/components/github/PRAssigneesPanel.tsx b/src/renderer/src/components/github/PRAssigneesPanel.tsx index d759d93880a..916c9f5d413 100644 --- a/src/renderer/src/components/github/PRAssigneesPanel.tsx +++ b/src/renderer/src/components/github/PRAssigneesPanel.tsx @@ -11,6 +11,7 @@ import { useRepoAssigneesBySlug } from '@/hooks/useGitHubSlugMetadata' import { getSettingsForRepoRuntimeOwner } from '@/lib/repo-runtime-owner' import { parseOwnerRepoFromItemUrl, + resolvePullRequestRepo, type GitHubWorkItemProjectOrigin } from '@/components/github/github-work-item-identity' import { runIssueUpdate } from '@/components/github/github-work-item-edit-mutations' @@ -82,6 +83,7 @@ export function PRAssigneesPanel({ ) const assigneeLogins = useMemo(() => localAssignees.map((user) => user.login), [localAssignees]) const assigneeSlug = useMemo(() => parseOwnerRepoFromItemUrl(item.url), [item.url]) + const prRepo = useMemo(() => resolvePullRequestRepo(item, projectOrigin), [item, projectOrigin]) const slugOwner = projectOrigin?.owner ?? assigneeSlug?.owner ?? null const slugRepo = projectOrigin?.repo ?? assigneeSlug?.repo ?? null const repoAssigneesBySlug = useRepoAssigneesBySlug( @@ -118,17 +120,24 @@ export function PRAssigneesPanel({ repoPath, sourceContext, projectOrigin, + issueRepo: prRepo, number: item.number, updates: isAssigned ? { removeAssignees: [login] } : { addAssignees: [login] } }), onOptimistic: () => { setLocalAssignees(nextAssignees) - patchWorkItem(item.id, { assignees: nextAssignees }, item.repoId, { sourceContext }) + patchWorkItem(item.id, { assignees: nextAssignees }, item.repoId, { + sourceContext, + ownerRepo: prRepo + }) patchProjectRowIfNeeded(nextLogins) }, onRevert: () => { setLocalAssignees(prevAssignees) - patchWorkItem(item.id, { assignees: prevAssignees }, item.repoId, { sourceContext }) + patchWorkItem(item.id, { assignees: prevAssignees }, item.repoId, { + sourceContext, + ownerRepo: prRepo + }) patchProjectRowIfNeeded(prevLogins) }, onSuccess: () => { @@ -147,6 +156,7 @@ export function PRAssigneesPanel({ onMutated, patchProjectRowIfNeeded, patchWorkItem, + prRepo, projectOrigin, repoPath, run, diff --git a/src/renderer/src/components/github/github-work-item-comment-mutations.ts b/src/renderer/src/components/github/github-work-item-comment-mutations.ts index f0f6fda3b82..6c7fc7e99ac 100644 --- a/src/renderer/src/components/github/github-work-item-comment-mutations.ts +++ b/src/renderer/src/components/github/github-work-item-comment-mutations.ts @@ -25,6 +25,7 @@ export function addIssueCommentForRepo(args: { repo: getGitHubRuntimeRepoId(args.sourceContext, args.repoId), number: args.number, body: args.body, + ...(args.type ? { type: args.type } : {}), prRepo: args.prRepo ?? null }, { timeoutMs: 30_000 } diff --git a/src/renderer/src/components/github/github-work-item-edit-mutations.ts b/src/renderer/src/components/github/github-work-item-edit-mutations.ts index 5c40aeb2328..543fb2395e8 100644 --- a/src/renderer/src/components/github/github-work-item-edit-mutations.ts +++ b/src/renderer/src/components/github/github-work-item-edit-mutations.ts @@ -29,19 +29,21 @@ export async function runIssueUpdate(args: { repoId?: string | null sourceContext?: TaskSourceContext | null projectOrigin: GitHubWorkItemProjectOrigin | undefined + issueRepo?: GitHubOwnerRepo | null number: number updates: Parameters[0]['updates'] }): Promise { - if (args.projectOrigin) { + const issueRepo = args.projectOrigin + if (issueRepo) { const targetSettings = args.sourceContext?.provider === 'github' ? getTaskSourceRuntimeSettings(args.sourceContext) : getGitHubMutationSettings(args.repoId) const target = getActiveRuntimeTarget(targetSettings) const updateArgs = { - owner: args.projectOrigin.owner, - repo: args.projectOrigin.repo, - host: githubProjectHost(args.projectOrigin.host), + owner: issueRepo.owner, + repo: issueRepo.repo, + host: githubProjectHost(issueRepo.host), number: args.number, updates: args.updates } @@ -59,18 +61,16 @@ export async function runIssueUpdate(args: { if (!res.ok) { throw new Error(res.error.message) } - if (target.kind === 'environment') { - notifyWorkItemDetailsMutation( - { - repoPath: args.repoPath ?? '', - repoId: args.repoId ?? undefined, - sourceContext: args.sourceContext, - type: 'issue', - number: args.number - }, - { local: false } - ) - } + notifyWorkItemDetailsMutation( + { + repoPath: args.repoPath ?? '', + repoId: args.repoId ?? undefined, + sourceContext: args.sourceContext, + type: 'issue', + number: args.number + }, + { local: target.kind !== 'environment' } + ) return } const runtimeHost = getGitHubSourceRuntimeHost(args.sourceContext) @@ -93,7 +93,8 @@ export async function runIssueUpdate(args: { repoId: args.repoId ?? undefined, sourceContext: args.sourceContext, number: args.number, - updates: args.updates + updates: args.updates, + ...(args.issueRepo ? { ownerRepo: args.issueRepo } : {}) }) if (!res.ok) { throw new Error(res.error) @@ -177,6 +178,7 @@ export async function runWorkItemBodyUpdate(args: { repoId: args.item.repoId, sourceContext: args.sourceContext, projectOrigin: args.projectOrigin, + issueRepo: args.parsedSlug, number: args.item.number, updates: { body: args.body } }) diff --git a/src/renderer/src/components/github/github-work-item-issue-target.test.ts b/src/renderer/src/components/github/github-work-item-issue-target.test.ts new file mode 100644 index 00000000000..88056a25cbb --- /dev/null +++ b/src/renderer/src/components/github/github-work-item-issue-target.test.ts @@ -0,0 +1,172 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { runIssueUpdate, runWorkItemBodyUpdate } from './github-work-item-edit-mutations' +import type { GitHubWorkItem } from '../../../../shared/github/work-item-types' +import type { TaskSourceContext } from '../../../../shared/task-source-context' +import { runGHEditLabelToggle } from '../github-item-dialog/edit-item-fields/gh-edit-section-mutations' + +vi.mock('@/store', () => ({ useAppStore: { getState: vi.fn() } })) +vi.mock('@/components/github/github-work-item-comment-mutations', () => ({ + notifyWorkItemDetailsMutation: vi.fn() +})) + +const ORIGIN = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } +const UPSTREAM = { owner: 'upstream-owner', repo: 'widgets', host: 'github.com' } +const projectOrigin = { + ...UPSTREAM, + number: 12, + type: 'issue' as const, + projectId: 'project-1', + projectItemId: 'project-item-12', + cacheKey: 'project-cache' +} +const localSource: TaskSourceContext = { + kind: 'task-source', + provider: 'github', + projectId: 'project-1', + hostId: 'local', + repoId: 'repo-1' +} +const item: GitHubWorkItem = { + id: 'issue:12', + type: 'issue', + number: 12, + title: 'Origin issue', + state: 'open', + url: 'https://github.com/fork-owner/widgets/issues/12', + labels: [], + updatedAt: '', + author: null, + repoId: 'repo-1' +} + +describe('issue edits retain the displayed repository', () => { + beforeEach(() => { + vi.stubGlobal('window', { + api: { + gh: { updateIssue: vi.fn().mockResolvedValue({ ok: true }), updateIssueBySlug: vi.fn() } + } + }) + }) + + it('keeps body edits on the registered repo path and passes the opened issue target', async () => { + await runWorkItemBodyUpdate({ + item, + repoPath: '/home/fixture/widgets', + projectOrigin: undefined, + body: 'Origin edit', + parsedSlug: ORIGIN + }) + + expect(window.api.gh.updateIssue).toHaveBeenCalledWith({ + repoId: 'repo-1', + repoPath: '/home/fixture/widgets', + sourceContext: undefined, + number: 12, + updates: { body: 'Origin edit' }, + ownerRepo: ORIGIN + }) + expect(window.api.gh.updateIssueBySlug).not.toHaveBeenCalled() + }) + + it('saves a Project row body in its upstream repository instead of the fork workspace', async () => { + vi.mocked(window.api.gh.updateIssueBySlug).mockResolvedValue({ ok: true }) + + await runWorkItemBodyUpdate({ + item: { ...item, url: 'https://github.com/upstream-owner/widgets/issues/12' }, + repoPath: '/home/fixture/fork-widgets', + sourceContext: localSource, + projectOrigin, + body: 'Project edit', + parsedSlug: UPSTREAM + }) + + expect(window.api.gh.updateIssueBySlug).toHaveBeenCalledWith({ + ...UPSTREAM, + number: 12, + updates: { body: 'Project edit' } + }) + expect(window.api.gh.updateIssue).not.toHaveBeenCalled() + }) + + it.each([ + { state: 'closed' as const }, + { addLabels: ['bug'] }, + { addAssignees: ['upstream-assignee'] } + ])('keeps Project field edits in the row repository: %j', async (updates) => { + vi.mocked(window.api.gh.updateIssueBySlug).mockResolvedValue({ ok: true }) + + await runIssueUpdate({ + repoPath: '/home/fixture/fork-widgets', + repoId: item.repoId, + sourceContext: localSource, + projectOrigin, + issueRepo: UPSTREAM, + number: 12, + updates + }) + + expect(window.api.gh.updateIssueBySlug).toHaveBeenCalledWith({ + ...UPSTREAM, + number: 12, + updates + }) + expect(window.api.gh.updateIssue).not.toHaveBeenCalled() + }) + + it.each([ + { state: 'closed' as const }, + { addLabels: ['bug'] }, + { addAssignees: ['fork-assignee'] } + ])('pins field edits to the opened issue repository: %j', async (updates) => { + await runIssueUpdate({ + repoPath: 'C:\\workspace\\widgets', + repoId: 'repo-1', + projectOrigin: undefined, + number: 12, + issueRepo: ORIGIN, + updates + }) + + expect(window.api.gh.updateIssue).toHaveBeenCalledWith( + expect.objectContaining({ + repoPath: 'C:\\workspace\\widgets', + ownerRepo: ORIGIN, + updates + }) + ) + expect(window.api.gh.updateIssueBySlug).not.toHaveBeenCalled() + }) + + it.each([ + { localLabels: [], updates: { addLabels: ['bug'] } }, + { localLabels: ['bug'], updates: { removeLabels: ['bug'] } } + ])( + 'retains the target for both label toggle directions: %j', + async ({ localLabels, updates }) => { + let mutation: Promise = Promise.resolve() + runGHEditLabelToggle({ + itemId: item.id, + itemNumber: item.number, + itemRepoId: item.repoId, + repoPath: 'C:\\workspace\\widgets', + projectOrigin: undefined, + issueRepo: ORIGIN, + label: 'bug', + localLabels, + run: async (_key, options) => { + mutation = options.mutate() + await mutation + }, + onLabelsChange: vi.fn(), + patchWorkItem: vi.fn(), + patchProjectRowIfNeeded: vi.fn(), + onMutated: vi.fn() + }) + await mutation + + expect(window.api.gh.updateIssue).toHaveBeenCalledWith( + expect.objectContaining({ ownerRepo: ORIGIN, updates }) + ) + } + ) +}) diff --git a/src/renderer/src/components/task-page-cache-selectors.test.ts b/src/renderer/src/components/task-page-cache-selectors.test.ts index 44777236862..ef1a0e20efe 100644 --- a/src/renderer/src/components/task-page-cache-selectors.test.ts +++ b/src/renderer/src/components/task-page-cache-selectors.test.ts @@ -204,8 +204,35 @@ describe('task page cache selectors', () => { } expect(findTaskPageDialogWorkItem(cache, null)).toBeNull() - expect(findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-1' })).toBe(item) - expect(findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-2' })).toBeNull() + expect( + findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-1', url: item.url }) + ).toBe(item) + expect( + findTaskPageDialogWorkItem(cache, { id: 'issue-1', repoId: 'repo-2', url: item.url }) + ).toBeNull() + }) + + it('keeps the clicked repository when a source change refreshes the same issue number', () => { + const origin = { + ...workItem('issue:12', 'repo-1'), + url: 'https://github.com/fork-owner/widgets/issues/12' + } + const upstream = { + ...origin, + url: 'https://github.com/upstream-owner/widgets/issues/12' + } + const clickedIssue = { id: origin.id, repoId: origin.repoId, url: origin.url } + const upstreamCache = { + upstream: entry([upstream]) + } + + expect(findTaskPageDialogWorkItem(upstreamCache, clickedIssue)).toBeNull() + expect( + findTaskPageDialogWorkItem( + { ...upstreamCache, origin: entry([origin]) }, + clickedIssue + ) + ).toBe(origin) }) it('reconciles paged table rows with patched work-item cache entries', () => { diff --git a/src/renderer/src/components/task-page-cache-selectors.ts b/src/renderer/src/components/task-page-cache-selectors.ts index a563dcae7b1..7cab1b34ece 100644 --- a/src/renderer/src/components/task-page-cache-selectors.ts +++ b/src/renderer/src/components/task-page-cache-selectors.ts @@ -28,6 +28,7 @@ export type TaskPageRepoCacheInput = { export type TaskPageDialogWorkItemKey = { id: string repoId: string + url: string } | null export type TaskPageRepoSourceState = { @@ -240,7 +241,10 @@ export function findTaskPageDialogWorkItem( for (const entry of Object.values(workItemsCache)) { const found = entry?.data?.find( - (wi) => wi.id === dialogWorkItemKey.id && wi.repoId === dialogWorkItemKey.repoId + (wi) => + wi.id === dialogWorkItemKey.id && + wi.repoId === dialogWorkItemKey.repoId && + wi.url === dialogWorkItemKey.url ) if (found) { return found diff --git a/src/renderer/src/components/task-page-github-confirmed-client-values.ts b/src/renderer/src/components/task-page-github-confirmed-client-values.ts new file mode 100644 index 00000000000..8b67b25d260 --- /dev/null +++ b/src/renderer/src/components/task-page-github-confirmed-client-values.ts @@ -0,0 +1,61 @@ +import type { GitHubOwnerRepo } from '../../../shared/github/pull-request-types' +import { githubRepoIdentityKey } from '../../../shared/github/repository-identity-key' +import { taskPageGitHubLastConfirmedKey } from './task-page-github-work-item-mutation-keys' + +const lastConfirmedClientValues = new Map< + string, + { value: unknown; ownerRepo?: GitHubOwnerRepo | null } +>() + +export function getLastConfirmedClientValue( + sourceScope: string | null, + repoId: string, + itemId: string, + family: string, + ownerRepo?: GitHubOwnerRepo | null +): unknown { + const entry = lastConfirmedClientValues.get( + taskPageGitHubLastConfirmedKey(sourceScope, repoId, itemId, family) + ) + if ( + ownerRepo !== undefined && + entry?.ownerRepo && + (!ownerRepo || githubRepoIdentityKey(ownerRepo) !== githubRepoIdentityKey(entry.ownerRepo)) + ) { + return undefined + } + return entry?.value +} +export function setLastConfirmedClientValue( + sourceScope: string | null, + repoId: string, + itemId: string, + family: string, + value: unknown, + ownerRepo?: GitHubOwnerRepo | null +): void { + lastConfirmedClientValues.set( + taskPageGitHubLastConfirmedKey(sourceScope, repoId, itemId, family), + { value, ownerRepo } + ) +} +export function deleteLastConfirmedClientValue( + sourceScope: string | null, + repoId: string, + itemId: string, + family: string, + ownerRepo?: GitHubOwnerRepo | null +): void { + if ( + ownerRepo !== undefined && + getLastConfirmedClientValue(sourceScope, repoId, itemId, family, ownerRepo) === undefined + ) { + return + } + lastConfirmedClientValues.delete( + taskPageGitHubLastConfirmedKey(sourceScope, repoId, itemId, family) + ) +} +export function clearLastConfirmedClientValues(): void { + lastConfirmedClientValues.clear() +} diff --git a/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts b/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts index b0bde81eb82..664b8bf7409 100644 --- a/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts +++ b/src/renderer/src/components/task-page-github-dialog-state-authority.test.ts @@ -184,4 +184,69 @@ describe('dialog state authority (STA-3343)', () => { expect(superseded.revert()).toBe(false) expect(getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state')).toBe('merged') }) + + it('holds qualified state only on its canonical repository and releases it on matching search', () => { + const ownerRepo = { owner: 'o', repo: 'r', host: 'github.com' } + setTaskPageGitHubMutationQueryKey('q') + assertTaskPageGitHubDialogStateAuthority({ + repoId: 'repo-1', + itemId: 'issue:1', + state: 'closed', + ownerRepo + }) + const other = item({ url: 'https://github.com/upstream/r/issues/1' }) + const enterprise = item({ url: 'https://ghe.example/o/r/issues/1' }) + expect( + applyPendingTaskPageGitHubMutationsToItems([item(), other, enterprise]).map( + (row) => row.state + ) + ).toEqual(['closed', 'open', 'open']) + adoptQuietSearchFieldsForItem({ + item: other, + serverItem: { ...other, state: 'closed' }, + sourceScope: null, + queryKey: 'q', + fetchStartedAtGeneration: getOrCreateQuietRevalidateState('q').dirtyGeneration, + patchWorkItem: () => {} + }) + expect(getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', ownerRepo)).toBe( + 'closed' + ) + adoptQuietSearchFieldsForItem({ + item: item(), + serverItem: item({ state: 'closed' }), + sourceScope: null, + queryKey: 'q', + fetchStartedAtGeneration: getOrCreateQuietRevalidateState('q').dirtyGeneration, + patchWorkItem: () => {} + }) + expect( + getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', ownerRepo) + ).toBeUndefined() + expect(applyPendingTaskPageGitHubMutationsToItems([item()])[0]?.state).toBe('open') + }) + + it('keeps another repository confirmation when an older qualified edit rolls back', () => { + const forkRepo = { owner: 'o', repo: 'r', host: 'github.com' } + const upstreamRepo = { ...forkRepo, owner: 'upstream' } + const forkAuthority = assertTaskPageGitHubDialogStateAuthority({ + repoId: 'repo-1', + itemId: 'issue:1', + state: 'closed', + ownerRepo: forkRepo + }) + assertTaskPageGitHubDialogStateAuthority({ + repoId: 'repo-1', + itemId: 'issue:1', + state: 'closed', + ownerRepo: upstreamRepo + }) + expect(forkAuthority.revert()).toBe(false) + expect(getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', upstreamRepo)).toBe( + 'closed' + ) + expect( + getLastConfirmedClientValue(null, 'repo-1', 'issue:1', 'state', forkRepo) + ).toBeUndefined() + }) }) diff --git a/src/renderer/src/components/task-page-github-dialog-state-authority.ts b/src/renderer/src/components/task-page-github-dialog-state-authority.ts index 2918e1eb4d0..55bb62aedf1 100644 --- a/src/renderer/src/components/task-page-github-dialog-state-authority.ts +++ b/src/renderer/src/components/task-page-github-dialog-state-authority.ts @@ -1,4 +1,5 @@ import type { GitHubWorkItem } from '../../../shared/github/work-item-types' +import type { GitHubOwnerRepo } from '../../../shared/github/pull-request-types' import { getTaskSourceCacheScope, type TaskSourceContext @@ -32,24 +33,57 @@ export function assertTaskPageGitHubDialogStateAuthority(args: { itemId: string state: GitHubWorkItem['state'] sourceContext?: TaskSourceContext | null + ownerRepo?: GitHubOwnerRepo | null }): { revert: () => boolean } { const sourceScope = args.sourceContext?.provider === 'github' ? getTaskSourceCacheScope(args.sourceContext) : null - const previous = getLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state') - setLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state', args.state) + const previous = getLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.ownerRepo + ) + setLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.state, + args.ownerRepo + ) markStateFamilyDirty(args.repoId, args.itemId) notifyTaskPageGitHubMutationRegistry() return { revert: () => { - const current = getLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state') + const current = getLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.ownerRepo + ) // A matching search adopt or newer mutation owns the state now. if (current !== args.state) { return false } if (previous === undefined) { - deleteLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state') + deleteLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + args.ownerRepo + ) } else { - setLastConfirmedClientValue(sourceScope, args.repoId, args.itemId, 'state', previous) + setLastConfirmedClientValue( + sourceScope, + args.repoId, + args.itemId, + 'state', + previous, + args.ownerRepo + ) } markStateFamilyDirty(args.repoId, args.itemId) notifyTaskPageGitHubMutationRegistry() diff --git a/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts b/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts index 6e98c29cb9b..d7387e6cfe4 100644 --- a/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts +++ b/src/renderer/src/components/task-page-github-work-item-mutation-composition.ts @@ -1,6 +1,7 @@ import type { ParsedTaskQuery } from '../../../shared/task-query' import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' import type { GitHubWorkItem } from '../../../shared/github/work-item-types' +import { parseGitHubIssueOrPRLink } from '../../../shared/github/links' import { recomputeTaskPageGitHubItemSoftHide, shouldSoftHideTaskPageGitHubWorkItem @@ -98,7 +99,13 @@ export function getRegistryMergedTaskPageGitHubWorkItem( // Why: after confirm, pending is cleared but search may still lag — hold the // last confirmed whole-field values until a matching adopt or newer pending. - const lastState = getLastConfirmedClientValue(sourceScope, item.repoId, item.id, 'state') + const lastState = getLastConfirmedClientValue( + sourceScope, + item.repoId, + item.id, + 'state', + parseGitHubIssueOrPRLink(item.url)?.slug ?? null + ) if (typeof lastState === 'string') { merged = { ...merged, state: lastState as GitHubWorkItem['state'] } } diff --git a/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts b/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts index b8e0ff01c83..1c50e9c547e 100644 --- a/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts +++ b/src/renderer/src/components/task-page-github-work-item-mutation-pages.ts @@ -1,5 +1,6 @@ import type { GitHubWorkItem } from '../../../shared/github/work-item-types' import type { TaskSourceContext } from '../../../shared/task-source-context' +import { parseGitHubIssueOrPRLink } from '../../../shared/github/links' import { getRegistryMergedTaskPageGitHubWorkItem } from './task-page-github-work-item-mutation-composition' import { getStickyHideEntry, @@ -74,7 +75,10 @@ export function reapplyPendingTaskPageGitHubMutationsToCache(args: { autoMergeEnabled: merged.autoMergeEnabled }, item.repoId, - { sourceContext: args.sourceContextByRepoId?.get(item.repoId) } + { + sourceContext: args.sourceContextByRepoId?.get(item.repoId), + ownerRepo: parseGitHubIssueOrPRLink(item.url)?.slug + } ) } } diff --git a/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts b/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts index 6988ff7f7b3..9fdf7760e19 100644 --- a/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts +++ b/src/renderer/src/components/task-page-github-work-item-mutation-registry.ts @@ -1,4 +1,17 @@ -import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' +import { + clearLastConfirmedClientValues, + deleteLastConfirmedClientValue, + getLastConfirmedClientValue, + setLastConfirmedClientValue as storeLastConfirmedClientValue +} from './task-page-github-confirmed-client-values' +export { + deleteLastConfirmedClientValue, + getLastConfirmedClientValue +} from './task-page-github-confirmed-client-values' +import type { + GitHubAssignableUser, + GitHubOwnerRepo +} from '../../../shared/github/pull-request-types' import type { PendingOp, StickyHideEntry, @@ -15,7 +28,6 @@ export type { import { serializeTaskPageGitHubMutationKey, taskPageGitHubItemKey, - taskPageGitHubLastConfirmedKey, taskPageGitHubSnapshotKey } from './task-page-github-work-item-mutation-keys' import { clearTaskPageGitHubQuietStates } from './task-page-github-work-item-quiet-state' @@ -37,7 +49,6 @@ const listeners = new Set() const pendingByKey = new Map() const generations = new Map() const confirmedSnapshots = new Map() -const lastConfirmedClientValues = new Map() /** * Why: after confirm, pending ops are gone but lastConfirmed/snapshots stay keyed * by sourceScope. Overlay must still resolve the same scope or authority is lost. @@ -79,7 +90,7 @@ export function getTaskPageGitHubConfirmedAuthorityItemKeys(): ReadonlySet, repoId?: string, - options?: { sourceContext?: TaskSourceContext | null } + options?: GitHubPatchWorkItemOptions ) => void export type BeginTaskPageGitHubWorkItemMutationArgs = { diff --git a/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts b/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts index 5547986ab2e..769948311d0 100644 --- a/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts +++ b/src/renderer/src/components/task-page-github-work-item-quiet-adopt.ts @@ -1,5 +1,6 @@ import type { GitHubWorkItem } from '../../../shared/github/work-item-types' import type { TaskSourceContext } from '../../../shared/task-source-context' +import { parseGitHubIssueOrPRLink } from '../../../shared/github/links' import { loginSetOfUsers, loginSetsEqual } from './task-page-github-work-item-mutation-patches' import { familiesFromPendingOp, @@ -52,6 +53,7 @@ export function adoptQuietSearchFieldsForItem(args: { }): { needTrailing: boolean } { const state = getOrCreateQuietRevalidateState(args.queryKey) const itemKey = taskPageGitHubItemKey(args.item.repoId, args.item.id) + const ownerRepo = parseGitHubIssueOrPRLink(args.item.url)?.slug ?? null let needTrailing = false const G0 = args.fetchStartedAtGeneration const tryFamily = ( @@ -91,7 +93,8 @@ export function adoptQuietSearchFieldsForItem(args: { 'state', () => { args.patchWorkItem(args.item.id, { state: args.serverItem.state }, args.item.repoId, { - sourceContext: args.sourceContext + sourceContext: args.sourceContext, + ownerRepo }) }, () => { @@ -99,14 +102,27 @@ export function adoptQuietSearchFieldsForItem(args: { args.sourceScope, args.item.repoId, args.item.id, - 'state' + 'state', + ownerRepo ) return last === undefined || args.serverItem.state === last }, () => - getLastConfirmedClientValue(args.sourceScope, args.item.repoId, args.item.id, 'state') !== - undefined, - () => deleteLastConfirmedClientValue(args.sourceScope, args.item.repoId, args.item.id, 'state') + getLastConfirmedClientValue( + args.sourceScope, + args.item.repoId, + args.item.id, + 'state', + ownerRepo + ) !== undefined, + () => + deleteLastConfirmedClientValue( + args.sourceScope, + args.item.repoId, + args.item.id, + 'state', + ownerRepo + ) ) tryFamily( 'autoMerge', @@ -115,7 +131,7 @@ export function adoptQuietSearchFieldsForItem(args: { args.item.id, { autoMergeEnabled: args.serverItem.autoMergeEnabled }, args.item.repoId, - { sourceContext: args.sourceContext } + { sourceContext: args.sourceContext, ownerRepo } ) }, () => { @@ -147,7 +163,7 @@ export function adoptQuietSearchFieldsForItem(args: { args.item.id, family === 'assignees' ? { assignees: serverList } : { reviewRequests: serverList }, args.item.repoId, - { sourceContext: args.sourceContext } + { sourceContext: args.sourceContext, ownerRepo } ) }, () => { diff --git a/src/renderer/src/components/use-task-page-github-detail.ts b/src/renderer/src/components/use-task-page-github-detail.ts index b2c61f64bde..655accb61d7 100644 --- a/src/renderer/src/components/use-task-page-github-detail.ts +++ b/src/renderer/src/components/use-task-page-github-detail.ts @@ -49,7 +49,8 @@ export function useTaskPageGitHubDetail(model: TaskPageGitHubListStateModel) { const dialogWorkItemKey = githubTaskDrawerWorkItem ? { id: githubTaskDrawerWorkItem.id, - repoId: githubTaskDrawerWorkItem.repoId + repoId: githubTaskDrawerWorkItem.repoId, + url: githubTaskDrawerWorkItem.url } : null const appliedWorkItemsCacheQuery = useMemo( @@ -67,7 +68,7 @@ export function useTaskPageGitHubDetail(model: TaskPageGitHubListStateModel) { ) ) - // Why: derive the dialog item from the cache for optimistic patches, falling back to the click-time snapshot for new stubs; key by repoId so same-number issues across repos resolve to the clicked row. + // Keep cache patches in the clicked conversation when origin and upstream share an issue number. const cachedDialogWorkItem = useAppStore((s) => findTaskPageDialogWorkItem(s.workItemsCache, dialogWorkItemKey) ) diff --git a/src/renderer/src/hooks/useGitHubRepoMetadata.ts b/src/renderer/src/hooks/useGitHubRepoMetadata.ts new file mode 100644 index 00000000000..8cdb20376bc --- /dev/null +++ b/src/renderer/src/hooks/useGitHubRepoMetadata.ts @@ -0,0 +1,95 @@ +import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' +import type { + GitHubAssignableUser, + GitHubOwnerRepo +} from '../../../shared/github/pull-request-types' +import { githubRepoIdentityKey } from '../../../shared/github/repository-identity-key' +import { createMetadataRequestStore } from './metadata-request-cache' +import { useMetadataListRequest, type MetadataListState } from './useMetadataListRequest' + +type GitHubMetadataOptions = { + runtimeEnvironmentId?: string | null + activeRuntimeEnvironmentId?: string | null + ownerRepo?: GitHubOwnerRepo | null +} + +const ghLabelStore = createMetadataRequestStore() +const ghAssigneeStore = createMetadataRequestStore() + +export function useRepoLabels( + repoPath: string | null, + repoId?: string | null, + options?: GitHubMetadataOptions +): MetadataListState { + const runtimeEnvironmentId = + options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null + const repoSelector = repoId ?? repoPath ?? '' + const ownerRepo = runtimeEnvironmentId ? null : options?.ownerRepo + const repositoryKey = ownerRepo + ? `${repoSelector}::${githubRepoIdentityKey(ownerRepo)}` + : repoSelector + const cacheKey = + repoPath || repoId + ? runtimeEnvironmentId + ? `runtime:${runtimeEnvironmentId}:${repoSelector}` + : repositoryKey + : null + + return useMetadataListRequest({ + cacheKey, + store: ghLabelStore, + errorFallback: 'Failed to load labels', + load: () => + runtimeEnvironmentId + ? callRuntimeRpc( + { kind: 'environment', environmentId: runtimeEnvironmentId }, + 'github.listLabels', + { repo: repoSelector }, + { timeoutMs: 15_000 } + ) + : window.api.gh.listLabels({ + repoPath: repoPath ?? '', + repoId: repoId ?? undefined, + ...(ownerRepo ? { ownerRepo } : {}) + }) + }) +} + +export function useRepoAssignees( + repoPath: string | null, + repoId?: string | null, + options?: GitHubMetadataOptions +): MetadataListState { + const runtimeEnvironmentId = + options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null + const repoSelector = repoId ?? repoPath ?? '' + const ownerRepo = runtimeEnvironmentId ? null : options?.ownerRepo + const repositoryKey = ownerRepo + ? `${repoSelector}::${githubRepoIdentityKey(ownerRepo)}` + : repoSelector + const cacheKey = + repoPath || repoId + ? runtimeEnvironmentId + ? `runtime:${runtimeEnvironmentId}:${repoSelector}` + : repositoryKey + : null + + return useMetadataListRequest({ + cacheKey, + store: ghAssigneeStore, + errorFallback: 'Failed to load assignees', + load: () => + runtimeEnvironmentId + ? callRuntimeRpc( + { kind: 'environment', environmentId: runtimeEnvironmentId }, + 'github.listAssignableUsers', + { repo: repoSelector }, + { timeoutMs: 15_000 } + ) + : window.api.gh.listAssignableUsers({ + repoPath: repoPath ?? '', + repoId: repoId ?? undefined, + ...(ownerRepo ? { ownerRepo } : {}) + }) + }) +} diff --git a/src/renderer/src/hooks/useIssueMetadata.test.tsx b/src/renderer/src/hooks/useIssueMetadata.test.tsx index bff14b1c091..f586e03fada 100644 --- a/src/renderer/src/hooks/useIssueMetadata.test.tsx +++ b/src/renderer/src/hooks/useIssueMetadata.test.tsx @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { clearLinearMetadataCache, useRepoLabels, + useRepoAssignees, useTeamStates, useTeamsStates } from './useIssueMetadata' @@ -17,7 +18,7 @@ const linearMocks = vi.hoisted(() => ({ })) const runtimeMocks = vi.hoisted(() => ({ callRuntimeRpc: vi.fn() })) -const githubMocks = vi.hoisted(() => ({ listLabels: vi.fn() })) +const githubMocks = vi.hoisted(() => ({ listLabels: vi.fn(), listAssignableUsers: vi.fn() })) vi.mock('@/runtime/runtime-linear-project-client', () => ({ linearTeamStates: linearMocks.linearTeamStates, @@ -38,7 +39,12 @@ const roots: Root[] = [] function installWindowApi(): void { Object.defineProperty(window, 'api', { configurable: true, - value: { gh: { listLabels: githubMocks.listLabels } } + value: { + gh: { + listLabels: githubMocks.listLabels, + listAssignableUsers: githubMocks.listAssignableUsers + } + } }) } @@ -67,6 +73,7 @@ describe('useIssueMetadata hooks', () => { linearMocks.linearTeamMembers.mockReset() runtimeMocks.callRuntimeRpc.mockReset() githubMocks.listLabels.mockReset() + githubMocks.listAssignableUsers.mockReset() installWindowApi() }) @@ -97,6 +104,70 @@ describe('useIssueMetadata hooks', () => { expect(runtimeMocks.callRuntimeRpc).not.toHaveBeenCalled() }) + it('keeps concurrent fork and upstream label lists separate when the fork reply is late', async () => { + const fork = { owner: 'fork', repo: 'widgets', host: 'github.com' } + const upstream = { owner: 'upstream', repo: 'widgets', host: 'github.com' } + let finishFork: (labels: string[]) => void = () => {} + const forkReply = new Promise((resolve) => { + finishFork = resolve + }) + githubMocks.listLabels.mockImplementation(({ ownerRepo }) => + ownerRepo.owner === 'fork' ? forkReply : Promise.resolve(['upstream-label']) + ) + let forkLabels: string[] = [] + let upstreamLabels: string[] = [] + function Probe(): null { + forkLabels = useRepoLabels(null, 'concurrent-folder', { ownerRepo: fork }).data + upstreamLabels = useRepoLabels(null, 'concurrent-folder', { ownerRepo: upstream }).data + return null + } + renderProbe() + await flushEffects() + expect(forkLabels).toEqual([]) + expect(upstreamLabels).toEqual(['upstream-label']) + await act(async () => { + finishFork(['fork-label']) + }) + await flushEffects() + expect(forkLabels).toEqual(['fork-label']) + expect(upstreamLabels).toEqual(['upstream-label']) + expect(githubMocks.listLabels).toHaveBeenCalledTimes(2) + }) + + it('ignores a late fork assignee reply after the opened issue switches to upstream', async () => { + const fork = { owner: 'fork', repo: 'widgets', host: 'github.com' } + const upstream = { owner: 'upstream', repo: 'widgets', host: 'github.com' } + let finishFork: (users: { login: string; name: null; avatarUrl: string }[]) => void = () => {} + const forkReply = new Promise<{ login: string; name: null; avatarUrl: string }[]>((resolve) => { + finishFork = resolve + }) + githubMocks.listAssignableUsers.mockImplementation(({ ownerRepo }) => + ownerRepo.owner === 'fork' + ? forkReply + : Promise.resolve([{ login: 'upstream-user', name: null, avatarUrl: '' }]) + ) + let logins: string[] = [] + function Probe({ ownerRepo }: { ownerRepo: typeof fork }): null { + logins = useRepoAssignees(null, 'switch-folder', { ownerRepo }).data.map((user) => user.login) + return null + } + renderProbe() + await flushEffects() + const root = roots.at(-1) + if (!root) { + throw new Error('Expected the rendered probe') + } + act(() => root.render()) + await flushEffects() + expect(logins).toEqual(['upstream-user']) + await act(async () => { + finishFork([{ login: 'fork-user', name: null, avatarUrl: '' }]) + }) + await flushEffects() + expect(logins).toEqual(['upstream-user']) + expect(githubMocks.listAssignableUsers).toHaveBeenCalledTimes(2) + }) + it('prefers an explicit remote environment and repo id', async () => { let labels: string[] = [] runtimeMocks.callRuntimeRpc.mockResolvedValue(['remote']) diff --git a/src/renderer/src/hooks/useIssueMetadata.ts b/src/renderer/src/hooks/useIssueMetadata.ts index a7e55b80700..2ce65c3055f 100644 --- a/src/renderer/src/hooks/useIssueMetadata.ts +++ b/src/renderer/src/hooks/useIssueMetadata.ts @@ -1,12 +1,11 @@ import { useEffect, useMemo, useRef, useState } from 'react' -import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' +import { getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' import { linearTeamLabels, linearTeamMembers, linearTeamStates } from '@/runtime/runtime-linear-project-client' import type { RuntimeLinearSettings } from '@/runtime/runtime-linear-client' -import type { GitHubAssignableUser } from '../../../shared/github/pull-request-types' import type { LinearLabel, LinearMember, @@ -23,79 +22,7 @@ import { } from './metadata-request-cache' import { useMetadataListRequest, type MetadataListState } from './useMetadataListRequest' -type GitHubMetadataOptions = { - runtimeEnvironmentId?: string | null - activeRuntimeEnvironmentId?: string | null -} - -const ghLabelStore = createMetadataRequestStore() -const ghAssigneeStore = createMetadataRequestStore() - -export function useRepoLabels( - repoPath: string | null, - repoId?: string | null, - options?: GitHubMetadataOptions -): MetadataListState { - const runtimeEnvironmentId = - options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null - const repoSelector = repoId ?? repoPath ?? '' - const cacheKey = - repoPath || repoId - ? runtimeEnvironmentId - ? `runtime:${runtimeEnvironmentId}:${repoSelector}` - : repoSelector - : null - - return useMetadataListRequest({ - cacheKey, - store: ghLabelStore, - errorFallback: 'Failed to load labels', - load: () => - runtimeEnvironmentId - ? callRuntimeRpc( - { kind: 'environment', environmentId: runtimeEnvironmentId }, - 'github.listLabels', - { repo: repoSelector }, - { timeoutMs: 15_000 } - ) - : window.api.gh - .listLabels({ repoPath: repoPath ?? '', repoId: repoId ?? undefined }) - .then((labels) => labels as string[]) - }) -} - -export function useRepoAssignees( - repoPath: string | null, - repoId?: string | null, - options?: GitHubMetadataOptions -): MetadataListState { - const runtimeEnvironmentId = - options?.runtimeEnvironmentId?.trim() || options?.activeRuntimeEnvironmentId?.trim() || null - const repoSelector = repoId ?? repoPath ?? '' - const cacheKey = - repoPath || repoId - ? runtimeEnvironmentId - ? `runtime:${runtimeEnvironmentId}:${repoSelector}` - : repoSelector - : null - - return useMetadataListRequest({ - cacheKey, - store: ghAssigneeStore, - errorFallback: 'Failed to load assignees', - load: () => - runtimeEnvironmentId - ? callRuntimeRpc( - { kind: 'environment', environmentId: runtimeEnvironmentId }, - 'github.listAssignableUsers', - { repo: repoSelector }, - { timeoutMs: 15_000 } - ) - : window.api.gh - .listAssignableUsers({ repoPath: repoPath ?? '', repoId: repoId ?? undefined }) - .then((users) => users as GitHubAssignableUser[]) - }) -} +export { useRepoLabels, useRepoAssignees } from './useGitHubRepoMetadata' const linearStateStore = createMetadataRequestStore() const linearLabelStore = createMetadataRequestStore() diff --git a/src/renderer/src/lib/github-work-item-source-lookup.test.ts b/src/renderer/src/lib/github-work-item-source-lookup.test.ts index 336583b1d6d..432401ce3e5 100644 --- a/src/renderer/src/lib/github-work-item-source-lookup.test.ts +++ b/src/renderer/src/lib/github-work-item-source-lookup.test.ts @@ -41,7 +41,8 @@ describe('GitHub source lookup routing', () => { repoId: 'renderer-repo', sourceContext: runtimeSourceContext, number: 42, - type: 'issue' + type: 'issue', + ownerRepo: { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } }) ).resolves.toBeNull() @@ -100,4 +101,22 @@ describe('GitHub source lookup routing', () => { }) expect(callRuntimeRpc).not.toHaveBeenCalled() }) + + it('sends a local issue row repository to the repo-scoped details route', async () => { + const ownerRepo = { owner: 'fork-owner', repo: 'widgets', host: 'github.com' } + vi.mocked(window.api.gh.workItemDetails).mockResolvedValue(null) + + await lookupGitHubWorkItemDetailsForSource({ + repoPath: '/home/fixture/widgets', + repoId: 'local-repo', + number: 12, + type: 'issue', + ownerRepo + }) + + expect(window.api.gh.workItemDetails).toHaveBeenCalledWith( + expect.objectContaining({ repoPath: '/home/fixture/widgets', ownerRepo }) + ) + expect(callRuntimeRpc).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/src/lib/github-work-item-source-lookup.ts b/src/renderer/src/lib/github-work-item-source-lookup.ts index 5a49e15ba83..da9e3775767 100644 --- a/src/renderer/src/lib/github-work-item-source-lookup.ts +++ b/src/renderer/src/lib/github-work-item-source-lookup.ts @@ -1,5 +1,6 @@ import type { GitHubWorkItem, GitHubWorkItemDetails } from '../../../shared/github/work-item-types' import type { TaskSourceContext } from '../../../shared/task-source-context' +import type { GitHubOwnerRepo } from '../../../shared/github/pull-request-types' import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import { getGitHubRuntimeRepoId, @@ -28,6 +29,7 @@ type GitHubWorkItemDetailsLookupArgs = { sourceContext?: TaskSourceContext | null number: number type: 'issue' | 'pr' + ownerRepo?: GitHubOwnerRepo | null } function runtimeRepoId(args: Pick): string { @@ -112,6 +114,7 @@ export function lookupGitHubWorkItemDetailsForSource( repoId: args.repoId, sourceContext, number: args.number, - type: args.type + type: args.type, + ...(args.ownerRepo ? { ownerRepo: args.ownerRepo } : {}) }) } diff --git a/src/renderer/src/store/github/cache-model.ts b/src/renderer/src/store/github/cache-model.ts index 27db1bf8167..d1546ff4768 100644 --- a/src/renderer/src/store/github/cache-model.ts +++ b/src/renderer/src/store/github/cache-model.ts @@ -61,4 +61,5 @@ export type ProjectRowContentPatch = { export type GitHubPatchWorkItemOptions = { sourceContext?: TaskSourceContext | null + ownerRepo?: GitHubOwnerRepo | null } diff --git a/src/renderer/src/store/github/work-item-mutation-actions.ts b/src/renderer/src/store/github/work-item-mutation-actions.ts index 97a6d3b66f3..fde46aef606 100644 --- a/src/renderer/src/store/github/work-item-mutation-actions.ts +++ b/src/renderer/src/store/github/work-item-mutation-actions.ts @@ -3,6 +3,8 @@ import type { AppState } from '../types' import type { GitHubSlice } from './slice-types' import { toast } from 'sonner' import type { GitHubWorkItem } from '../../../../shared/github/work-item-types' +import { parseGitHubIssueOrPRLink } from '../../../../shared/github/links' +import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' import { getTaskSourceCacheScope } from '../../../../shared/task-source-context' import { translate } from '@/i18n/i18n' import { getSettingsForRepoRuntimeOwner } from '@/lib/repo-runtime-owner' @@ -24,6 +26,7 @@ export const createWorkItemMutationActions = ( options?.sourceContext?.provider === 'github' ? getTaskSourceCacheScope(options.sourceContext) : null + const repositoryKey = options?.ownerRepo ? githubRepoIdentityKey(options.ownerRepo) : null for (const key of Object.keys(nextCache)) { // Why: don't patch another host/account's visually identical issue/PR cache entry. if (sourceScope && key !== sourceScope && !key.startsWith(`${sourceScope}::`)) { @@ -34,9 +37,18 @@ export const createWorkItemMutationActions = ( continue } // Why: issue/PR ids are only unique within a repo; cross-repo views can share `pr:42`. - const idx = entry.data.findIndex( - (item) => item.id === itemId && (!repoId || item.repoId === repoId) - ) + const idx = entry.data.findIndex((item) => { + if (item.id !== itemId || (repoId && item.repoId !== repoId)) { + return false + } + if (!repositoryKey) { + return true + } + const itemRepository = parseGitHubIssueOrPRLink(item.url)?.slug + return ( + itemRepository !== undefined && githubRepoIdentityKey(itemRepository) === repositoryKey + ) + }) if (idx === -1) { continue } diff --git a/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts b/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts index 77f997083aa..6fe2a9e068a 100644 --- a/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts +++ b/src/renderer/src/store/slices/github-work-item-cache-identity.test.ts @@ -111,6 +111,43 @@ describe('createGitHubSlice.patchWorkItem', () => { }) expect(secondPatched).toBe(secondItem) }) + + it('scopes a canonical repository patch within its existing host and account scope', () => { + const store = createTestStore() + const source = githubSourceContext('local', 'repo-1') + const otherAccount = { ...source, projectHostSetupId: 'another-account-setup' } + const canonical: GitHubWorkItem = { + id: 'issue:42', + repoId: 'repo-1', + type: 'issue', + number: 42, + title: 'Fork issue', + state: 'open', + labels: [], + updatedAt: '', + author: null, + url: 'https://github.com/fork/widgets/issues/42' + } + const upstream = { ...canonical, url: 'https://github.com/upstream/widgets/issues/42' } + const enterprise = { ...canonical, url: 'https://ghe.example:8443/fork/widgets/issues/42' } + const sourceKey = workItemsCacheKey('repo-1', 20, '', getTaskSourceCacheScope(source)) + const otherKey = workItemsCacheKey('repo-1', 20, '', getTaskSourceCacheScope(otherAccount)) + store.setState({ + workItemsCache: { + [sourceKey]: { data: [upstream, enterprise, canonical], fetchedAt: 1 }, + [otherKey]: { data: [canonical], fetchedAt: 1 } + } + }) + store.getState().patchWorkItem(canonical.id, { labels: ['bug'] }, canonical.repoId, { + sourceContext: source, + ownerRepo: { owner: 'FORK', repo: 'Widgets', host: ' GitHub.com ' } + }) + const rows = store.getState().workItemsCache[sourceKey]?.data + expect(rows?.[0]).toBe(upstream) + expect(rows?.[1]).toBe(enterprise) + expect(rows?.[2].labels).toEqual(['bug']) + expect(store.getState().workItemsCache[otherKey]?.data?.[0]).toBe(canonical) + }) }) describe('createGitHubSlice.fetchWorkItems cache identity', () => { From 94c446afb3e32f074c94bf717280a655272e2afc Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 3 Oct 2026 23:19:26 -0700 Subject: [PATCH 19/23] fix(native-chat): a rejected /compact stays in the chat, said once A /compact the host recorded and then rejected was hidden, so on several paths (blocked after the reply stopped waiting, rejected on restart, or seen from another window or the phone) it vanished with nothing saying it failed. It now stays as a not-sent row for every viewer, like any message the host recorded. Its line says only "Your message was not sent." when a loaded host row already says why: its turn's result row (found by the command's id) or the failed start's row. The sender's command reply no longer repeats it when the loaded journal shows the host recorded the command under the operation id, and the text stays the row's rather than coming back to the composer. The desktop pane's delivery-notice wiring moves into its own hook. --- .../mobile-native-chat-unsent-notices.test.ts | 36 +++++ .../mobile-native-chat-unsent-notices.ts | 9 +- ...mobile-structured-composer-command.test.ts | 26 ++++ .../mobile-structured-composer-command.ts | 17 ++- ...use-mobile-structured-send-with-outcome.ts | 6 + .../structured-agent-session-command-turn.ts | 3 +- .../NativeChatStructuredSession.tsx | 52 ++----- ...ructured-agent-session-delivery-notices.ts | 16 +- ...d-agent-session-message-projection.test.ts | 137 ++++++++++++------ ...ructured-conversation-command-send.test.ts | 33 ++++- .../structured-conversation-command-send.ts | 7 + ...tured-agent-session-command-result-rows.ts | 18 +++ ...ent-session-command-start-failure.test.tsx | 31 +++- ...ructured-agent-session-delivery-notices.ts | 53 +++++++ .../use-structured-agent-session-mutate.ts | 8 +- .../use-structured-agent-session.ts | 6 + .../structured-agent-session-command-entry.ts | 25 ++++ ...ctured-agent-session-message-projection.ts | 47 ++++-- ...-agent-session-recorded-rejection-words.ts | 15 +- 19 files changed, 420 insertions(+), 125 deletions(-) create mode 100644 src/renderer/src/components/native-chat/use-structured-agent-session-command-result-rows.ts create mode 100644 src/renderer/src/components/native-chat/use-structured-agent-session-delivery-notices.ts diff --git a/mobile/src/session/mobile-native-chat-unsent-notices.test.ts b/mobile/src/session/mobile-native-chat-unsent-notices.test.ts index 5f4b503e06d..485159a856b 100644 --- a/mobile/src/session/mobile-native-chat-unsent-notices.test.ts +++ b/mobile/src/session/mobile-native-chat-unsent-notices.test.ts @@ -9,6 +9,7 @@ import type { AgentJournalRenderItem, AgentJournalSubmission } from '../../../src/shared/agent-session-journal-types' +import { structuredAgentSessionCommandResultRowIdentity } from '../../../src/shared/structured-agent-session-command-entry' import { structuredAgentSessionStartFailureRowIdentity } from '../../../src/shared/structured-agent-session-start-failure-row-key' import { mobileNativeChatUnsentNotices } from './mobile-native-chat-unsent-notices' @@ -62,6 +63,41 @@ describe('the line under a message the host recorded and did not deliver', () => ) }) + // Its turn's result row says how a refused command ended; a blocked one has only its own line. + it("says only that a command was not sent when its result row says why, else the command's words", () => { + const providerRejected: AgentSessionFailureFact = { + kind: 'providerRejected', + detail: { text: 'busy', audience: 'person' } + } + const refused = rejected({ clientMessageId: 'compact-1', rejection: providerRejected }) + const resultRow: AgentJournalRenderItem = { + itemId: agentJournalItemKey(structuredAgentSessionCommandResultRowIdentity('compact-1')), + revision: 1, + sequence: 2, + observedAt: 2, + body: { + kind: 'status', + tone: 'error', + ...agentSessionFailureWords( + { kind: 'compactionFailed', detail: { text: 'busy', audience: 'person' } }, + { agentName: 'Codex', surface: 'row' } + ) + } + } + const blocked = rejected({ + clientMessageId: 'compact-2', + rejection: { kind: 'commandRefused' } + }) + const notices = mobileNativeChatUnsentNotices( + { items: [resultRow], submissions: [refused, blocked] }, + 'codex' + ) + expect(notices.get(agentJournalSubmissionKey('compact-1'))).toBe('Your message was not sent.') + expect(notices.get(agentJournalSubmissionKey('compact-2'))).toBe( + "This command didn't run. Try it again." + ) + }) + it('has nothing to say when no send was rejected', () => { expect( mobileNativeChatUnsentNotices( diff --git a/mobile/src/session/mobile-native-chat-unsent-notices.ts b/mobile/src/session/mobile-native-chat-unsent-notices.ts index d04cc894012..b8f76a876a4 100644 --- a/mobile/src/session/mobile-native-chat-unsent-notices.ts +++ b/mobile/src/session/mobile-native-chat-unsent-notices.ts @@ -3,6 +3,7 @@ import { formatAgentTypeLabel } from '../../../src/shared/agent-type-label' import { agentJournalSubmissionKey } from '../../../src/shared/agent-session-journal-item-key' +import { structuredAgentSessionCommandResultRows } from '../../../src/shared/structured-agent-session-command-entry' import { agentSessionWriteNoticeEnglish } from '../../../src/shared/agent-session-refusal-notice' import type { NativeChatTurnJournal } from '../../../src/shared/native-chat-turn-membership' import { @@ -22,6 +23,7 @@ export function mobileNativeChatUnsentNotices( return NO_NOTICES } const startFailures = structuredAgentSessionStartFailureFacts(journal.items) + const commandResults = structuredAgentSessionCommandResultRows(journal.items) const context = { retryControl: false, ...(agent ? { agentName: formatAgentTypeLabel(agent) } : {}) @@ -30,7 +32,12 @@ export function mobileNativeChatUnsentNotices( rejected.map((submission) => [ agentJournalSubmissionKey(submission.clientMessageId), agentSessionWriteNoticeEnglish( - structuredAgentSessionRecordedRejectionParts(submission, context, startFailures) + structuredAgentSessionRecordedRejectionParts( + submission, + context, + startFailures, + commandResults + ) ) ]) ) diff --git a/mobile/src/session/mobile-structured-composer-command.test.ts b/mobile/src/session/mobile-structured-composer-command.test.ts index 8a54140a7dc..f60bab8486c 100644 --- a/mobile/src/session/mobile-structured-composer-command.test.ts +++ b/mobile/src/session/mobile-structured-composer-command.test.ts @@ -33,6 +33,7 @@ function setup() { conversationCommands: ['clear', 'compact'] }, canRun: () => true, + recorded: () => false, onError: vi.fn(), timeoutMs: 15000 } @@ -119,6 +120,31 @@ describe('mobile structured conversation commands', () => { expect(fields.command).toBe('clear') expect(asyncStorage.setItem).not.toHaveBeenCalled() }) + // Its row in the chat says it was not sent and why, for every viewer, so no banner repeats it. + it('banners a refused /compact only when the journal does not show the host recorded it', async () => { + const refused = { + ok: true, + result: { + ok: true, + value: { command: 'compact', state: 'completed', error: "This command didn't run." } + } + } + const { input, sendRequest } = setup() + sendRequest.mockResolvedValue(refused) + const sentId = () => { + const envelope = requestFields(sendRequest.mock.calls.at(-1)).envelope + return typeof envelope === 'object' && envelope !== null && 'clientOperationId' in envelope + ? envelope.clientOperationId + : undefined + } + input.recorded = (clientMessageId) => clientMessageId === sentId() + expect(await dispatchMobileStructuredCommand(input)).toBe('accepted') + expect(input.onError).not.toHaveBeenCalled() + + input.recorded = () => false + expect(await dispatchMobileStructuredCommand(input)).toBe('rejected') + expect(input.onError).toHaveBeenCalledWith("This command didn't run.") + }) it('keeps ordinary messages on the existing send path', async () => { const { input, sendRequest } = setup() expect(await dispatchMobileStructuredCommand({ ...input, text: 'hello' })).toBeNull() diff --git a/mobile/src/session/mobile-structured-composer-command.ts b/mobile/src/session/mobile-structured-composer-command.ts index ebe30fe8803..83ccd8fe263 100644 --- a/mobile/src/session/mobile-structured-composer-command.ts +++ b/mobile/src/session/mobile-structured-composer-command.ts @@ -7,6 +7,7 @@ import { import type { RpcClient } from '../transport/rpc-client' import type { MobileNativeChatSendOutcome } from './mobile-native-chat-send' import { requestStructuredAgentSessionMutation } from './mobile-structured-agent-session-rpc' +import { structuredSessionOperationId } from './structured-session-operation-id' export async function dispatchMobileStructuredCommand(input: { text: string @@ -17,6 +18,8 @@ export async function dispatchMobileStructuredCommand(input: { pending: { current: boolean } controller: StructuredAgentSessionComposerOptions canRun: () => boolean + /** Whether the loaded journal shows the message the host recorded under this id. */ + recorded: (clientMessageId: string) => boolean onError: (message: string) => void timeoutMs: number }): Promise { @@ -41,6 +44,7 @@ export async function dispatchMobileStructuredCommand(input: { } } input.pending.current = true + const clientOperationId = structuredSessionOperationId() try { const result = await requestStructuredAgentSessionMutation({ @@ -50,6 +54,7 @@ export async function dispatchMobileStructuredCommand(input: { method: 'agentSession.conversationCommand', fingerprintMethod: 'agentSession.conversationCommand', fields: { command }, + clientOperationId, timeoutMs: Math.max(input.timeoutMs, 195_000) }) if ( @@ -62,9 +67,15 @@ export async function dispatchMobileStructuredCommand(input: { error: 'Conversation operation was not confirmed.' } } - return result.status === 'accepted' - ? { accepted: !result.value.error, error: result.value.error ?? null } - : { accepted: false, error: result.message } + if (result.status !== 'accepted') { + return { accepted: false, error: result.message } + } + // The host answered for a command it recorded: its row in the chat says how it went, so + // no banner repeats it and the text stays the row's, not the composer's. + if (result.value.state === 'completed' && input.recorded(clientOperationId)) { + return { accepted: true, error: null } + } + return { accepted: !result.value.error, error: result.value.error ?? null } } finally { input.pending.current = false } diff --git a/mobile/src/session/use-mobile-structured-send-with-outcome.ts b/mobile/src/session/use-mobile-structured-send-with-outcome.ts index 44c94f777a2..1ea04fb8c7f 100644 --- a/mobile/src/session/use-mobile-structured-send-with-outcome.ts +++ b/mobile/src/session/use-mobile-structured-send-with-outcome.ts @@ -10,6 +10,7 @@ import { type StructuredAgentSessionAttachment } from '../../../src/shared/structured-agent-session-outbox' import type { StructuredAgentSessionComposerOptions } from '../../../src/shared/structured-agent-session-composer' +import { structuredAgentSessionJournalShowsSubmission } from '../../../src/shared/structured-agent-session-message-projection' import type { StructuredAgentSessionState } from '../../../src/shared/structured-agent-session-reducer' import type { RpcClient } from '../transport/rpc-client' import type { MobileNativeChatSendOutcome } from './mobile-native-chat-send' @@ -98,6 +99,11 @@ export function useMobileStructuredSendWithOutcome(args: { !stateRef.current.items.some( (item) => pendingStructuredApproval(item) || pendingStructuredQuestion(item) ), + recorded: (clientMessageId) => + structuredAgentSessionJournalShowsSubmission( + stateRef.current.submissions, + clientMessageId + ), onError: onSendError, timeoutMs }) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-command-turn.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-command-turn.ts index 9eb5cb02ea4..ec8cde5aec0 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-command-turn.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-command-turn.ts @@ -31,6 +31,7 @@ import type { AgentSessionConversationCommand } from '../../../shared/agent-sess import type { AgentSessionRecord } from '../../../shared/agent-session-record' import type { AgentChildWorkView } from '../../../shared/agent-status-child-work-view' import { agentSessionRefusalReference } from '../../../shared/agent-session-wire-refusals' +import { structuredAgentSessionCommandResultRowIdentity } from '../../../shared/structured-agent-session-command-entry' import { agentJournalTurnBody, readAgentJournalTurn @@ -102,7 +103,7 @@ export function structuredAgentSessionCommandTurn(clientMessageId: string): { identity, itemId: agentJournalItemKey(identity), turnId: `compact:${clientMessageId}`, - resultIdentity: { provider: 'orca', clientMessageId: `command-result:${clientMessageId}` } + resultIdentity: structuredAgentSessionCommandResultRowIdentity(clientMessageId) } } diff --git a/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx b/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx index 48e24f4d464..7ab28caab1e 100644 --- a/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx +++ b/src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { useMemo, useRef, useState } from 'react' import { agentSessionPromptQuestions } from '../../../../shared/agent-session-question-answer' import { dispatchStructuredAgentSessionComposerCommand } from '../../../../shared/structured-agent-session-composer' import { structuredAgentSessionPaneKey } from '../../../../shared/structured-agent-session-projection' @@ -28,11 +28,7 @@ import { useAppStore } from '../../store' import { structuredAgentLabel } from '@/lib/structured-agent-session-launch-label' import { NativeChatThreadGoalBanner } from './NativeChatThreadGoalBanner' import { structuredAgentSessionReadFailureNotice } from './structured-agent-session-read-failure-notice' -import { useStructuredAgentSessionStartFailureFacts } from './use-structured-agent-session-start-failure-facts' -import { structuredAgentSessionDeliveryNotices } from './structured-agent-session-delivery-notices' -import type { AgentJournalSubmission } from '../../../../shared/agent-session-journal-types' - -const NO_SUBMISSIONS: readonly AgentJournalSubmission[] = [] +import { useStructuredAgentSessionDeliveryNotices } from './use-structured-agent-session-delivery-notices' export function NativeChatStructuredSession( props: Omit @@ -113,42 +109,16 @@ export function NativeChatStructuredSession( }), [controller, props.agent, props.sessionId] ) - // Read at click time, so the notices stay put while the outbox's Retry is rebuilt each render. - const retryRef = useRef(controller.retry) - useEffect(() => { - retryRef.current = controller.retry - }) - const retryDelivery = useCallback((clientMessageId: string) => { - retryRef.current(clientMessageId) - }, []) const agentLabel = structuredAgentLabel(props.agent === 'codex' ? 'codex' : 'claude') - // Only a row shown as not sent reads the journal's rows, so a new batch of them re-renders no row - // else. Read from the transcript, not this window's outbox: the host's record alone shows one. - const hasRejected = controller.messages.some((message) => message.unsent === true) - const rejectionRows = hasRejected ? controller.submissions : NO_SUBMISSIONS - const startFailures = useStructuredAgentSessionStartFailureFacts( - controller.journalItems, - hasRejected - ) - const deliveryNotices = useMemo( - () => - structuredAgentSessionDeliveryNotices( - controller.outbox, - agentLabel, - retryDelivery, - rejectionRows, - startFailures, - controller.failedHere - ), - [ - controller.outbox, - agentLabel, - retryDelivery, - rejectionRows, - startFailures, - controller.failedHere - ] - ) + const deliveryNotices = useStructuredAgentSessionDeliveryNotices({ + messages: controller.messages, + journalItems: controller.journalItems, + submissions: controller.submissions, + outbox: controller.outbox, + failedHere: controller.failedHere, + retry: controller.retry, + agentLabel + }) const viewState = selectNativeChatViewState(session, { readRetries: true }) const readFailure = controller.status === 'error' diff --git a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts index 51a391b584b..acaf5227c99 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-delivery-notices.ts @@ -8,10 +8,11 @@ // queue moves. // // A message the host recorded and then rejected is worded from the journal's own fact, found by id; -// the message keeps only a smaller copy, read when its submission is not loaded. A rejection that -// is a failed start's, the fact its loaded row states, says only that it was not sent: the row -// already says why. One no outbox entry here carries (another client's send, or one whose entry is -// gone) is worded from the journal alone, with no Retry: this client holds nothing to send. +// the message keeps only a smaller copy, read when its submission is not loaded. A rejection a +// loaded host row already states (a failed start's, or a command's result row) says only that it +// was not sent: the row already says why. One no outbox entry here carries (another client's +// send, or one whose entry is gone) is worded from the journal alone, with no Retry: this client +// holds nothing to send. import { readWholeAgentSessionFailureFact, @@ -106,7 +107,9 @@ export function structuredAgentSessionDeliveryNotices( /** What the loaded start-failure rows state, from `structuredAgentSessionStartFailureFacts`. */ startFailures: readonly AgentSessionFailureFact[], /** Ids whose send failed or was refused while this chat was open: only they word their cause. */ - failedHere: ReadonlySet + failedHere: ReadonlySet, + /** Commands whose loaded result row says how they ended, from `structuredAgentSessionCommandResultRows`. */ + commandResults?: ReadonlySet ): ReadonlyMap { const admission = admitStructuredAgentSessionOutboxEntry(outbox) const held = admission.state === 'blocked' ? admission.entry.clientMessageId : null @@ -148,7 +151,8 @@ export function structuredAgentSessionDeliveryNotices( structuredAgentSessionRecordedRejectionParts( submission, { agentName, retryControl: false }, - startFailures + startFailures, + commandResults ) ) }) diff --git a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts index 27a0d68584c..de97a002234 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-message-projection.test.ts @@ -10,6 +10,13 @@ import { agentJournalSubmissionKey } from '../../../../shared/agent-session-journal-item-key' import { agentJournalTurnBody } from '../../../../shared/agent-session-turn-record' +import { + structuredAgentSessionCommandResultRowIdentity, + structuredAgentSessionCommandResultRows +} from '../../../../shared/structured-agent-session-command-entry' +import { structuredAgentSessionStartFailureFacts } from '../../../../shared/structured-agent-session-recorded-rejection-words' +import { structuredAgentSessionStartFailureRowIdentity } from '../../../../shared/structured-agent-session-start-failure-row-key' +import { structuredAgentSessionDeliveryNotices } from './structured-agent-session-delivery-notices' import { projectStructuredAgentSessionMessages as projectForPhone } from '../../../../shared/structured-agent-session-message-projection' import { createStructuredAgentSessionOutboxEntry } from '../../../../shared/structured-agent-session-outbox' import { projectStructuredAgentSessionMessages } from './structured-agent-session-message-projection' @@ -122,31 +129,60 @@ describe('structured agent session message projection', () => { ]) }) - // The command's reply (blocked) or its turn's result row (refused) already says it failed. + // A rejected command is the host's like any send: its row stays, and its line says why only + // where no loaded host row already does. The sender's reply stays quiet (command-send tests). const busy = { text: 'thread busy', audience: 'person' as const } + const startFailed: AgentSessionFailureFact = { kind: 'providerStartFailed' } it.each<{ name: string rejection: AgentSessionFailureFact - resultRow?: AgentSessionFailureFact + row?: 'result' | 'start' + line: string }>([ - { name: 'blocked at handover', rejection: { kind: 'commandRefused' } }, + { + name: 'blocked at handover', + rejection: { kind: 'commandRefused' }, + line: "This command didn't run. Try it again." + }, { name: 'refused by the provider', rejection: { kind: 'providerRejected', detail: busy }, - resultRow: { kind: 'compactionFailed', detail: busy } + row: 'result', + line: 'Your message was not sent.' + }, + { + name: 'rejected by a failed start', + rejection: startFailed, + row: 'start', + line: 'Your message was not sent.' } - ])( - 'says a /compact $name failed only where the command reports it', - ({ rejection, resultRow }) => { - const opensTurn = resultRow !== undefined - const compact = { - ...submission(0), - dispatchState: 'rejected' as const, - providerItemId: null, - rejection + ])('keeps a /compact $name, said once', ({ rejection, row, line }) => { + const compact = { + ...submission(0), + dispatchState: 'rejected' as const, + providerItemId: null, + rejection + } + const userItemId = agentJournalSubmissionKey(compact.clientMessageId) + const turnItemId = agentJournalItemKey({ provider: 'orca', clientMessageId: 'command-turn:c' }) + const hostRow = ( + itemId: string, + fact: AgentSessionFailureFact, + turnScope?: AgentJournalRenderItem['turnScope'] + ): AgentJournalRenderItem => ({ + itemId, + revision: 1, + sequence: 2, + observedAt: 2, + ...(turnScope ? { turnScope } : {}), + body: { + kind: 'status', + tone: 'error', + ...agentSessionFailureWords(fact, { agentName: 'Codex', surface: 'row' }) } - const userItemId = agentJournalSubmissionKey(compact.clientMessageId) - const commandItem: AgentJournalRenderItem = { + }) + const items: AgentJournalRenderItem[] = [ + { itemId: userItemId, revision: 1, sequence: 0, @@ -157,16 +193,8 @@ describe('structured agent session message projection', () => { blocks: [{ type: 'text', text: '/compact' }], command: { name: 'compact' } } - } - const turnItemId = agentJournalItemKey({ - provider: 'orca', - clientMessageId: 'command-turn:c' - }) - const resultId = agentJournalItemKey({ - provider: 'orca', - clientMessageId: 'command-result:c' - }) - const turnRows: AgentJournalRenderItem[] = resultRow + }, + ...(row === 'result' ? [ { itemId: turnItemId, @@ -183,31 +211,44 @@ describe('structured agent session message projection', () => { completedAt: 2 }) }, - { - itemId: resultId, - revision: 1, - sequence: 2, - observedAt: 2, - turnScope: { kind: 'turn', turnItemId }, - body: { - kind: 'status', - tone: 'error', - ...agentSessionFailureWords(resultRow, { agentName: 'Codex', surface: 'row' }) - } - } + hostRow( + agentJournalItemKey( + structuredAgentSessionCommandResultRowIdentity(compact.clientMessageId) + ), + { kind: 'compactionFailed', detail: busy }, + { kind: 'turn', turnItemId } + ) ] - : [] - const items = [commandItem, ...turnRows] - for (const messages of [ - projectStructuredAgentSessionMessages(items, [], [compact]), - projectForPhone(items, [], [compact]) - ]) { - expect(messages.find((message) => message.id === userItemId)).toBeUndefined() - expect(messages.some((message) => message.unsent === true)).toBe(false) - expect(messages.some((message) => message.id === resultId)).toBe(opensTurn) - } + : []), + ...(row === 'start' + ? [ + hostRow( + agentJournalItemKey(structuredAgentSessionStartFailureRowIdentity('g')), + startFailed + ) + ] + : []) + ] + for (const messages of [ + projectStructuredAgentSessionMessages(items, [], [compact]), + projectForPhone(items, [], [compact]) + ]) { + expect(messages.filter((message) => message.unsent === true).map((m) => m.id)).toEqual([ + userItemId + ]) + expect(messages).toHaveLength(row ? 2 : 1) } - ) + const notices = structuredAgentSessionDeliveryNotices( + [], + 'Codex', + () => {}, + [compact], + structuredAgentSessionStartFailureFacts(items), + new Set(), + structuredAgentSessionCommandResultRows(items) + ) + expect(notices.get(userItemId)?.text).toBe(line) + }) // The host sends a queued draft under a fresh id per hand-off; its card keeps the text meanwhile. it('leaves a rejected queued-draft hand-off to its card: one bubble once a later hand-off lands', () => { diff --git a/src/renderer/src/components/native-chat/structured-conversation-command-send.test.ts b/src/renderer/src/components/native-chat/structured-conversation-command-send.test.ts index 498e9d4fc01..b448c3c1396 100644 --- a/src/renderer/src/components/native-chat/structured-conversation-command-send.test.ts +++ b/src/renderer/src/components/native-chat/structured-conversation-command-send.test.ts @@ -53,7 +53,8 @@ function hostResult( async function sent( result: AgentSessionConversationCommandResult, - provider: 'claude' | 'codex' = 'claude' + provider: 'claude' | 'codex' = 'claude', + recordedIds: readonly string[] = [] ) { return sendStructuredConversationCommand({ command: result.command, @@ -61,7 +62,8 @@ async function sent( pending: { current: false }, blocked: false, startFailures: () => [], - send: async () => ({ kind: 'done', value: result }) + recorded: (clientMessageId) => recordedIds.includes(clientMessageId), + send: async () => ({ kind: 'done', value: result, operationId: 'op-command' }) }) } @@ -98,6 +100,30 @@ describe('the line under the composer after a conversation command failed', () = ) }) + // Its row in the chat says it was not sent and why, for every viewer, so the reply stays quiet. + it('says nothing for a command the host recorded, and leaves its text to its row', async () => { + for (const result of [ + hostResult('compact', { kind: 'commandRefused' }), + hostResult('compact', { + kind: 'providerRejected', + detail: { text: 'busy', audience: 'person' } + }), + hostResult('compact', START_FAILED) + ]) { + expect(await sent(result, 'codex', ['op-command'])).toEqual({ accepted: true, error: null }) + } + // Not answered yet: nothing on its row says how it went. + const notStarted: AgentSessionConversationCommandResult = { + command: 'compact', + state: 'unknown', + error: 'The command has not started yet.' + } + expect(await sent(notStarted, 'codex', ['op-command'])).toEqual({ + accepted: false, + error: 'The command has not started yet.' + }) + }) + it("says it in the reader's language", async () => { await i18n.changeLanguage('fr') expect((await sent(hostResult('clear', START_FAILED))).error).toBe( @@ -156,7 +182,8 @@ describe('the line under the composer after a conversation command failed', () = pending: { current: false }, blocked: false, startFailures: () => [START_FAILED], - send: async () => ({ kind: 'done', value: result }) + recorded: () => false, + send: async () => ({ kind: 'done', value: result, operationId: 'op-command' }) }) ).toEqual({ accepted: false, error }) }) diff --git a/src/renderer/src/components/native-chat/structured-conversation-command-send.ts b/src/renderer/src/components/native-chat/structured-conversation-command-send.ts index 666715d265b..adf08feb2af 100644 --- a/src/renderer/src/components/native-chat/structured-conversation-command-send.ts +++ b/src/renderer/src/components/native-chat/structured-conversation-command-send.ts @@ -21,6 +21,8 @@ export async function sendStructuredConversationCommand(input: { blocked: boolean /** What the chat's loaded start-failure rows state, read when the reply lands. */ startFailures: () => readonly AgentSessionFailureFact[] + /** Whether the loaded journal shows the message the host recorded under this id. */ + recorded: (clientMessageId: string) => boolean send: ( command: AgentSessionConversationCommand ) => Promise> @@ -45,6 +47,11 @@ export async function sendStructuredConversationCommand(input: { return { accepted: false, error: null } } const { value } = outcome + // The host answered for a command it recorded: its row in the chat says how it went, so the + // reply says nothing more, and the text is the row's, not the composer's. + if (value.state === 'completed' && input.recorded(outcome.operationId)) { + return { accepted: true, error: null } + } // The chat's own start failed and its loaded row already says why, as for a message that start // rejected. A /clear's failed start is its new chat's, whose row this pane never shows, and a // command this build doesn't know may be either, so its host's words are shown. diff --git a/src/renderer/src/components/native-chat/use-structured-agent-session-command-result-rows.ts b/src/renderer/src/components/native-chat/use-structured-agent-session-command-result-rows.ts new file mode 100644 index 00000000000..a526c2c227a --- /dev/null +++ b/src/renderer/src/components/native-chat/use-structured-agent-session-command-result-rows.ts @@ -0,0 +1,18 @@ +import { useMemo } from 'react' +import type { AgentJournalRenderItem } from '../../../../shared/agent-session-journal-types' +import { structuredAgentSessionCommandResultRows } from '../../../../shared/structured-agent-session-command-entry' + +const NO_ROWS: ReadonlySet = new Set() + +/** The commands whose result row is loaded, read only while `enabled`. Held while unchanged, so a + * streaming turn does not rebuild every row's delivery notice. */ +export function useStructuredAgentSessionCommandResultRows( + items: readonly AgentJournalRenderItem[], + enabled: boolean +): ReadonlySet { + const key = useMemo( + () => (enabled ? [...structuredAgentSessionCommandResultRows(items)].sort().join('\0') : ''), + [enabled, items] + ) + return useMemo(() => (key === '' ? NO_ROWS : new Set(key.split('\0'))), [key]) +} diff --git a/src/renderer/src/components/native-chat/use-structured-agent-session-command-start-failure.test.tsx b/src/renderer/src/components/native-chat/use-structured-agent-session-command-start-failure.test.tsx index 698d83e6106..14225571465 100644 --- a/src/renderer/src/components/native-chat/use-structured-agent-session-command-start-failure.test.tsx +++ b/src/renderer/src/components/native-chat/use-structured-agent-session-command-start-failure.test.tsx @@ -5,7 +5,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { AgentSessionFailureFact } from '../../../../shared/agent-session-failure' import { agentSessionFailureWords } from '../../../../shared/agent-session-failure-words' import { agentJournalItemKey } from '../../../../shared/agent-session-journal-item-key' -import type { AgentJournalRenderItem } from '../../../../shared/agent-session-journal-types' +import type { + AgentJournalRenderItem, + AgentJournalSubmission +} from '../../../../shared/agent-session-journal-types' import { structuredAgentSessionStartFailureRowIdentity } from '../../../../shared/structured-agent-session-start-failure-row-key' const mocks = vi.hoisted(() => ({ @@ -15,6 +18,7 @@ const mocks = vi.hoisted(() => ({ })) let fence = 3 let items: AgentJournalRenderItem[] = [] +let submissions: AgentJournalSubmission[] = [] vi.mock('sonner', () => ({ toast: { error: mocks.toastError, message: vi.fn() } })) @@ -28,7 +32,7 @@ vi.mock('./use-structured-agent-session-read', () => ({ state: { fence, items, - submissions: [], + submissions, status: 'ready', error: null, hasOlder: false, @@ -150,6 +154,7 @@ beforeEach(() => { vi.clearAllMocks() fence = 3 items = [] + submissions = [] }) afterEach(async () => { @@ -204,6 +209,26 @@ describe('a conversation command whose reply lands after the fence moved', () => ).toEqual({ accepted: false, error: null }) }) + // The host recorded the /compact under the operation id, so its row in the chat says it. + it('says nothing for a /compact the host recorded, and clears its text: its row holds it', async () => { + submissions = [ + { + clientMessageId: 'operation-1', + fence: 3, + payloadFingerprint: 'fingerprint', + dispatchState: 'rejected', + providerItemId: null, + reason: 'busy', + rejection: { kind: 'commandRefused' }, + submittedAt: 1, + resolvedAt: 1 + } + ] + expect( + await commandAcrossFenceMove('compact', commandReply('compact', { kind: 'commandRefused' })) + ).toEqual({ accepted: true, error: null }) + }) + it("says why a /compact's start failed while its row is not loaded", async () => { expect( await commandAcrossFenceMove('compact', commandReply('compact', RESTART_FAILED)) @@ -284,7 +309,7 @@ describe('a conversation command whose reply lands after the fence moved', () => await Promise.all([older, newer]) }) expect(await older).toEqual({ kind: 'dropped' }) - expect(await newer).toEqual({ kind: 'done', value: commandReply('clear').value }) + expect(await newer).toMatchObject({ kind: 'done', value: commandReply('clear').value }) }) it('drops a reply once the pane has moved to another chat or closed', async () => { diff --git a/src/renderer/src/components/native-chat/use-structured-agent-session-delivery-notices.ts b/src/renderer/src/components/native-chat/use-structured-agent-session-delivery-notices.ts new file mode 100644 index 00000000000..3b943dfc921 --- /dev/null +++ b/src/renderer/src/components/native-chat/use-structured-agent-session-delivery-notices.ts @@ -0,0 +1,53 @@ +import { useCallback, useEffect, useMemo, useRef } from 'react' +import type { + AgentJournalRenderItem, + AgentJournalSubmission +} from '../../../../shared/agent-session-journal-types' +import type { NativeChatMessage } from '../../../../shared/native-chat-types' +import type { StructuredAgentSessionOutboxEntry } from '../../../../shared/structured-agent-session-outbox' +import type { NativeChatDeliveryNotice } from './NativeChatMessageRow' +import { structuredAgentSessionDeliveryNotices } from './structured-agent-session-delivery-notices' +import { useStructuredAgentSessionCommandResultRows } from './use-structured-agent-session-command-result-rows' +import { useStructuredAgentSessionStartFailureFacts } from './use-structured-agent-session-start-failure-facts' + +const NO_SUBMISSIONS: readonly AgentJournalSubmission[] = [] + +/** The structured chat's delivery notices, keyed by the message id each row renders under. */ +export function useStructuredAgentSessionDeliveryNotices(input: { + messages: readonly NativeChatMessage[] + journalItems: readonly AgentJournalRenderItem[] + submissions: readonly AgentJournalSubmission[] + outbox: readonly StructuredAgentSessionOutboxEntry[] + failedHere: ReadonlySet + retry: (clientMessageId: string) => void + agentLabel: string +}): ReadonlyMap { + const { outbox, failedHere, agentLabel } = input + // Read at click time, so the notices stay put while the outbox's Retry is rebuilt each render. + const retryRef = useRef(input.retry) + useEffect(() => { + retryRef.current = input.retry + }) + const retry = useCallback((clientMessageId: string) => { + retryRef.current(clientMessageId) + }, []) + // Only a row shown as not sent reads the journal's rows, so a new batch of them re-renders no row + // else. Read from the transcript, not this window's outbox: the host's record alone shows one. + const hasNotSent = input.messages.some((message) => message.unsent === true) + const rejectionRows = hasNotSent ? input.submissions : NO_SUBMISSIONS + const startFailures = useStructuredAgentSessionStartFailureFacts(input.journalItems, hasNotSent) + const commandResults = useStructuredAgentSessionCommandResultRows(input.journalItems, hasNotSent) + return useMemo( + () => + structuredAgentSessionDeliveryNotices( + outbox, + agentLabel, + retry, + rejectionRows, + startFailures, + failedHere, + commandResults + ), + [outbox, agentLabel, retry, rejectionRows, startFailures, failedHere, commandResults] + ) +} diff --git a/src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts b/src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts index 1f5116718a5..9aaf350f6ce 100644 --- a/src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts +++ b/src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts @@ -27,7 +27,8 @@ import { structuredSessionOperationId } from './use-structured-agent-session-out import { agentSessionWriteFailureText } from './agent-session-write-notice-text' export type StructuredAgentSessionWriteOutcome = - | { kind: 'done'; value: T } + /** `operationId`: the id the host recorded this write under. */ + | { kind: 'done'; value: T; operationId: string } /** Refused or failed, with what to tell the person. */ | { kind: 'not-done'; notice: string } /** Settled for an owner or session this pane no longer shows; there is nothing to say. */ @@ -105,12 +106,13 @@ export function useStructuredAgentSessionMutate(args: { } return enabledRef.current && (stateRef.current.fence === targetFence || waitedOn) } + const clientOperationId = structuredSessionOperationId() let result: AgentSessionMutationResult try { result = await callStructuredAgentSession>(target, method, { envelope: { sessionId, - clientOperationId: structuredSessionOperationId(), + clientOperationId, expectedRuntimeFence: targetFence, payloadFingerprint: structuredAgentSessionPayloadFingerprint({ method: fingerprintMethod, @@ -148,7 +150,7 @@ export function useStructuredAgentSessionMutate(args: { if (!settlesHere()) { return { kind: 'dropped' } } - return { kind: 'done', value: result.value } + return { kind: 'done', value: result.value, operationId: clientOperationId } }, [enabled, sessionId, stateRef, target] ) diff --git a/src/renderer/src/components/native-chat/use-structured-agent-session.ts b/src/renderer/src/components/native-chat/use-structured-agent-session.ts index 0f10c1ded2b..57a28bafed1 100644 --- a/src/renderer/src/components/native-chat/use-structured-agent-session.ts +++ b/src/renderer/src/components/native-chat/use-structured-agent-session.ts @@ -37,6 +37,7 @@ import { useStructuredAgentSessionRailOutline } from './use-structured-agent-ses import { useStructuredAgentSessionQueuedMessages } from './use-structured-agent-session-queued-messages' import { outboxOutsideQueuedCards } from './structured-agent-session-queued-cards' import { structuredAgentSessionStartFailureFacts } from '../../../../shared/structured-agent-session-recorded-rejection-words' +import { structuredAgentSessionJournalShowsSubmission } from '../../../../shared/structured-agent-session-message-projection' import { hostStatesTurnScopes } from '../../../../shared/native-chat-turn-membership' export type { StructuredPromptItem } from './structured-agent-session-message-projection' @@ -191,6 +192,11 @@ export function useStructuredAgentSession(args: { outbox.length ), startFailures: () => structuredAgentSessionStartFailureFacts(stateRef.current.items), + recorded: (clientMessageId) => + structuredAgentSessionJournalShowsSubmission( + stateRef.current.submissions, + clientMessageId + ), send: (command) => write( 'agentSession.conversationCommand', diff --git a/src/shared/structured-agent-session-command-entry.ts b/src/shared/structured-agent-session-command-entry.ts index fd76b45d43a..0da091c388b 100644 --- a/src/shared/structured-agent-session-command-entry.ts +++ b/src/shared/structured-agent-session-command-entry.ts @@ -2,13 +2,17 @@ // request of the user's to the agent: the sidebar's prompt, preview and verdict, the completion // feed and restart resume all read past them to the last real request. +import { parseAgentJournalItemKey } from './agent-session-journal-item-key' import type { AgentJournalItemBody, + AgentJournalItemIdentity, AgentJournalRenderItem, AgentJournalTurnLifecycle } from './agent-session-journal-types' import { readAgentJournalTurn } from './agent-session-turn-record' +const COMMAND_RESULT_ROW = 'command-result:' + export function isStructuredAgentSessionCommandEntry( body: AgentJournalItemBody | null | undefined ): boolean { @@ -52,3 +56,24 @@ export function isStructuredAgentSessionCommandRow( (scope?.kind === 'turn' && commandTurnItemIds.has(scope.turnItemId)) ) } + +/** The one row a command's turn reports how it ended on, keyed by the command's message id. */ +export function structuredAgentSessionCommandResultRowIdentity( + clientMessageId: string +): Extract { + return { provider: 'orca', clientMessageId: `${COMMAND_RESULT_ROW}${clientMessageId}` } +} + +/** The message ids of the commands whose result row is in `items`. */ +export function structuredAgentSessionCommandResultRows( + items: readonly AgentJournalRenderItem[] +): Set { + const ids = new Set() + for (const item of items) { + const identity = parseAgentJournalItemKey(item.itemId) + if (identity?.provider === 'orca' && identity.clientMessageId.startsWith(COMMAND_RESULT_ROW)) { + ids.add(identity.clientMessageId.slice(COMMAND_RESULT_ROW.length)) + } + } + return ids +} diff --git a/src/shared/structured-agent-session-message-projection.ts b/src/shared/structured-agent-session-message-projection.ts index 7de8a923efd..8757341d8e2 100644 --- a/src/shared/structured-agent-session-message-projection.ts +++ b/src/shared/structured-agent-session-message-projection.ts @@ -8,9 +8,39 @@ import type { StructuredAgentSessionOutboxEntry } from './structured-agent-sessi import { structuredAgentSessionEntryHeldForRetry } from './structured-agent-session-outbox-admission' import { reconcileStructuredAgentSessionOutboxWithQueue } from './structured-agent-session-draft-hand-off' import { dispatchWasWithdrawn } from './structured-agent-session-dispatch-rejection' -import { isStructuredAgentSessionCommandEntry } from './structured-agent-session-command-entry' import { projectStructuredItemsToNativeChat } from './structured-agent-session-projection' +/** + * Whether the host's record of a send keeps it in the conversation, for every viewer. A send the + * host recorded and then did not deliver stays, shown as not sent: dropping the sender's outbox + * entry can never make it vanish. It leaves only where something else owns it: the user withdrew + * it with Stop, or its queued draft's card keeps the text. + */ +export function structuredAgentSessionRecordStaysInChat( + submission: Pick< + AgentJournalSubmission, + 'dispatchState' | 'queuedMessageId' | 'reason' | 'rejection' + > +): boolean { + return ( + submission.dispatchState !== 'rejected' || + (!dispatchWasWithdrawn(submission) && submission.queuedMessageId === undefined) + ) +} + +/** Whether the loaded journal draws the send recorded under `clientMessageId` in the chat, where + * its row, not a reply, says how it went. */ +export function structuredAgentSessionJournalShowsSubmission( + submissions: readonly AgentJournalSubmission[], + clientMessageId: string +): boolean { + return submissions.some( + (submission) => + submission.clientMessageId === clientMessageId && + structuredAgentSessionRecordStaysInChat(submission) + ) +} + export function projectStructuredAgentSessionMessages( items: readonly AgentJournalRenderItem[], outbox: readonly StructuredAgentSessionOutboxEntry[], @@ -18,10 +48,6 @@ export function projectStructuredAgentSessionMessages( projectItems = projectStructuredItemsToNativeChat ): NativeChatMessage[] { const optimistic = reconcileStructuredAgentSessionOutboxWithQueue(outbox, submissions) - // A send the host recorded and then did not deliver stays in the conversation from the host's - // record, shown as not sent, for every viewer: dropping the sender's outbox entry can never make - // it vanish. It leaves only where something else owns it: the user withdrew it with Stop, its - // queued draft's card keeps the text, or it is a command whose reply or result row says it failed. const notSent = new Set() const ownedElsewhere = new Set() for (const submission of submissions) { @@ -29,19 +55,16 @@ export function projectStructuredAgentSessionMessages( continue } const key = agentJournalSubmissionKey(submission.clientMessageId) - if (dispatchWasWithdrawn(submission) || submission.queuedMessageId !== undefined) { - ownedElsewhere.add(key) - } else { + if (structuredAgentSessionRecordStaysInChat(submission)) { notSent.add(key) + } else { + ownedElsewhere.add(key) } } const visibleItems: AgentJournalRenderItem[] = [] const refused = new Map() for (const item of items) { - if ( - ownedElsewhere.has(item.itemId) || - (notSent.has(item.itemId) && isStructuredAgentSessionCommandEntry(item.body)) - ) { + if (ownedElsewhere.has(item.itemId)) { refused.set(item.itemId, item) } else { visibleItems.push(item) diff --git a/src/shared/structured-agent-session-recorded-rejection-words.ts b/src/shared/structured-agent-session-recorded-rejection-words.ts index dd3dfe724b6..e57928cf480 100644 --- a/src/shared/structured-agent-session-recorded-rejection-words.ts +++ b/src/shared/structured-agent-session-recorded-rejection-words.ts @@ -1,6 +1,6 @@ // What a send the host recorded and then did not deliver says on its own row, on every client. -// A rejection that is a failed start's, the fact its loaded row states, says only that it was not -// sent: the row already says why. +// A rejection a loaded host row already states says only that it was not sent: a failed start's, +// the fact its row states, or a command's, whose turn's result row says how it ended. import { readAgentSessionFailureFact, @@ -61,13 +61,20 @@ export function agentSessionFailureStatedByStartRow( ) } +const NO_COMMAND_RESULTS: ReadonlySet = new Set() + /** The words for a recorded rejection that no outbox entry on this client carries. */ export function structuredAgentSessionRecordedRejectionParts( submission: AgentJournalSubmission, context: AgentSessionFailureWordsContext, - startFailures: readonly AgentSessionFailureFact[] + startFailures: readonly AgentSessionFailureFact[], + /** Commands whose result row is loaded, from `structuredAgentSessionCommandResultRows`. */ + commandResults: ReadonlySet = NO_COMMAND_RESULTS ): AgentSessionWriteNoticePart[] { - if (agentSessionFailureStatedByStartRow(submission.rejection, startFailures)) { + if ( + commandResults.has(submission.clientMessageId) || + agentSessionFailureStatedByStartRow(submission.rejection, startFailures) + ) { return agentSessionWriteNotDoneParts('send') } return structuredAgentSessionAttemptFailureParts( From 7ebec5b869c966b0b2403fec7c5b3ef0d4a66e07 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 4 Oct 2026 00:00:20 -0700 Subject: [PATCH 20/23] perf(native-chat): read only status rows when finding command result rows The scan parsed every journal key on each update while a row showed as not sent, about 3-17 ms per call at 3000 items. Every result row is a status row, so the scan now skips the rest before parsing (about 0.02-0.04 ms). --- src/shared/structured-agent-session-command-entry.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/shared/structured-agent-session-command-entry.ts b/src/shared/structured-agent-session-command-entry.ts index 0da091c388b..d0eb38b4ba0 100644 --- a/src/shared/structured-agent-session-command-entry.ts +++ b/src/shared/structured-agent-session-command-entry.ts @@ -70,6 +70,10 @@ export function structuredAgentSessionCommandResultRows( ): Set { const ids = new Set() for (const item of items) { + // Every result row is a status row; parsing only those keeps a long chat's scan cheap. + if (item.body.kind !== 'status') { + continue + } const identity = parseAgentJournalItemKey(item.itemId) if (identity?.provider === 'orca' && identity.clientMessageId.startsWith(COMMAND_RESULT_ROW)) { ids.add(identity.clientMessageId.slice(COMMAND_RESULT_ROW.length)) From f6784ad53bb0c84051eca43a3c7b28ab0e9f58df Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 4 Oct 2026 00:00:21 -0700 Subject: [PATCH 21/23] fix(mobile): resend the same text as a new message when its kept id was rejected After a lost answer, the phone keeps that message's id for its text. If the host since recorded and rejected it, sending the same text again replayed the id, which answered with the same rejection: no banner, no bubble, and an empty composer, so the press did nothing. Such a replay now goes out under a fresh id, as a withdrawn one already does. "Sent, but this phone couldn't update its record" is no longer said for a resend the host recorded and rejected; its row says it was not sent. --- ...ructured-agent-session-send-bypass.test.ts | 56 ++++++++++++++++++- .../mobile-structured-agent-session-send.ts | 54 ++++++++++++------ 2 files changed, 90 insertions(+), 20 deletions(-) diff --git a/mobile/src/session/mobile-structured-agent-session-send-bypass.test.ts b/mobile/src/session/mobile-structured-agent-session-send-bypass.test.ts index c9da99a3cba..8631e61809d 100644 --- a/mobile/src/session/mobile-structured-agent-session-send-bypass.test.ts +++ b/mobile/src/session/mobile-structured-agent-session-send-bypass.test.ts @@ -31,8 +31,33 @@ function queuedAnswer(clientMessageId: string, state: 'waiting' | 'withdrawn'): }) } -/** Each `agentSession.send` answered in turn: a lost answer, or a queued draft in that state. */ -function hostAnswering(answers: readonly ('lost' | 'waiting' | 'withdrawn')[]) { +function submissionAnswer(clientMessageId: string, dispatchState: 'accepted' | 'rejected') { + return ok({ + ok: true, + replayed: false, + fence: 3, + cursor: { epoch: 'epoch-1', sequence: 1 }, + value: { + clientMessageId, + submission: { + clientMessageId, + fence: 3, + payloadFingerprint: 'fingerprint', + dispatchState, + providerItemId: null, + reason: dispatchState === 'rejected' ? 'provider_write_failed: broken pipe' : null, + submittedAt: 1, + resolvedAt: 1 + } + } + }) +} + +/** Each `agentSession.send` answered in turn: a lost answer, a queued draft in that state, or a + * recorded submission in that state. */ +function hostAnswering( + answers: readonly ('lost' | 'waiting' | 'withdrawn' | 'accepted' | 'rejected')[] +) { const ids: string[] = [] const deliveries: unknown[] = [] const sendRequest = vi.fn(async (_method, params) => { @@ -44,7 +69,9 @@ function hostAnswering(answers: readonly ('lost' | 'waiting' | 'withdrawn')[]) { if (answer === 'lost' || answer === undefined) { throw markRpcDeliveryUnknown(new Error('Connection closed')) } - return queuedAnswer(id, answer) + return answer === 'accepted' || answer === 'rejected' + ? submissionAnswer(id, answer) + : queuedAnswer(id, answer) }) const client: RpcClient = { sendRequest, @@ -154,4 +181,27 @@ describe('a resend past a saved record storage would not clear', () => { "Sent, but this phone couldn't update its record of sent messages." ) }) + + // Its row says it was not sent, so replaying the kept id would answer that again and do nothing. + it('sends the same text as a new message when its kept id answers as recorded and rejected', async () => { + const { client, ids } = hostAnswering(['lost', 'rejected', 'accepted']) + const onError = vi.fn() + expect(await sendAgain(client, onError, false)).toBe('unknown') + expect(await sendAgain(client, onError, false)).toBe('accepted') + expect(ids).toHaveLength(3) + expect(ids[1]).toBe(ids[0]) + expect(ids[2]).not.toBe(ids[0]) + expect(onError).not.toHaveBeenCalled() + }) + + it('never says a resend went out when the host recorded and rejected it', async () => { + const { client, ids } = hostAnswering(['lost', 'withdrawn', 'rejected']) + const onError = vi.fn() + expect(await sendAgain(client, onError)).toBe('unknown') + asyncStorage.setItem.mockRejectedValue(new Error('disk full')) + asyncStorage.removeItem.mockRejectedValue(new Error('disk full')) + expect(await sendAgain(client, onError)).toBe('queued') + expect(ids).toHaveLength(3) + expect(onError).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/session/mobile-structured-agent-session-send.ts b/mobile/src/session/mobile-structured-agent-session-send.ts index ce7f60a8b92..8ad04187d63 100644 --- a/mobile/src/session/mobile-structured-agent-session-send.ts +++ b/mobile/src/session/mobile-structured-agent-session-send.ts @@ -11,10 +11,12 @@ import type { RpcClient } from '../transport/rpc-client' import type { MobileNativeChatSendOutcome } from './mobile-native-chat-send' import { requestStructuredAgentSessionMutation, - timeoutForDeadline + timeoutForDeadline, + type StructuredAgentSessionMutationCallResult } from './mobile-structured-agent-session-rpc' import { structuredSessionOperationId } from './structured-session-operation-id' import { mobileStructuredSendDelivery } from './mobile-structured-send-delivery' +import { dispatchWasWithdrawn } from '../../../src/shared/structured-agent-session-dispatch-rejection' import { bypassedMobileStructuredSendOperationId, clearMobileStructuredSendOperation, @@ -36,13 +38,13 @@ export async function sendMobileStructuredAgentSessionMessage(input: { delivery?: 'queue-if-active' deadline?: number onError: (message: string) => void - /** Internal: the one fresh-id resend after a withdrawn replay. */ - resendingAfterWithdrawal?: true - /** Internal: that resend when the withdrawn id's record could not be cleared. It bypasses the + /** Internal: the one fresh-id resend after a replay that settled without delivering. */ + resendingAfterSettledReplay?: true + /** Internal: that resend when the settled id's record could not be cleared. It bypasses the * record, so failing storage never blocks the send; its id is kept in memory for this app run, * so a retry after a lost answer replays it rather than sending again. */ bypassRetainedRecord?: true - /** Internal: on that resend, the key the withdrawn record matched. The remembered id is keyed + /** Internal: on that resend, the key the settled record matched. The remembered id is keyed * by it, so a capability change since the lost answer never mints another id. */ resendOperationKey?: string }): Promise { @@ -165,22 +167,22 @@ export async function sendMobileStructuredAgentSessionMessage(input: { result.status === 'accepted' && 'queued' in result.value && result.value.queued?.state === 'withdrawn' - if (withdrawnReplay && operation.retained && !input.resendingAfterWithdrawal) { - // The retained id's draft was withdrawn, so it never reached the agent: - // this identical message is a new one, not a replay to swallow. A record - // storage would not clear is bookkeeping: it is reported, never allowed to - // block the send. - const resent = await sendMobileStructuredAgentSessionMessage({ + if ( + operation.retained && + !input.resendingAfterSettledReplay && + // A recorded rejection's row already says it was not sent, so a replay would do nothing. + (withdrawnReplay || recordedRejection(result)) + ) { + // The retained id's message never reached the agent (its draft was withdrawn, or the host + // recorded and rejected it): this identical message is a new one, not a replay to swallow. A + // record storage would not clear is bookkeeping: it is reported, never allowed to block the + // send. + return sendMobileStructuredAgentSessionMessage({ ...input, - resendingAfterWithdrawal: true, + resendingAfterSettledReplay: true, resendOperationKey: operationKey, ...(released ? {} : { bypassRetainedRecord: true as const }) }) - // Only when the resend is known to have gone out; an unconfirmed one may not have. - if (!released && (resent === 'accepted' || resent === 'queued')) { - input.onError("Sent, but this phone couldn't update its record of sent messages.") - } - return resent } if (withdrawnReplay) { // Not resent: no card and no bubble holds the text, so it goes back to the @@ -191,6 +193,13 @@ export async function sendMobileStructuredAgentSessionMessage(input: { if (outcome.error !== null) { input.onError(outcome.error) } + // Only once the resend is known to have gone out: an unconfirmed one may not have, and one the + // host recorded and rejected shows as not sent. + const wentOut = + outcome.outcome === 'accepted' || (outcome.outcome === 'queued' && !recordedRejection(result)) + if (input.bypassRetainedRecord && wentOut) { + input.onError("Sent, but this phone couldn't update its record of sent messages.") + } return outcome.outcome } @@ -240,3 +249,14 @@ async function adoptBypassedOperation(input: { return bypassOperation(input.operationKey, input.payloadFingerprint, input.attachmentPaths) } } + +/** An answer that the host recorded the message and then rejected it, not by a Stop. */ +function recordedRejection( + result: StructuredAgentSessionMutationCallResult +): boolean { + const submission = + result.status === 'accepted' && 'submission' in result.value + ? result.value.submission + : undefined + return submission?.dispatchState === 'rejected' && !dispatchWasWithdrawn(submission) +} From b1b78b4d20b3664b1f3948a923664a2e7ba517f4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 4 Oct 2026 07:06:01 +0000 Subject: [PATCH 22/23] Update README downloads badge --- docs/assets/readme-downloads.svg | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/assets/readme-downloads.svg b/docs/assets/readme-downloads.svg index fdf7ecb03a0..3f3cf232ad1 100644 --- a/docs/assets/readme-downloads.svg +++ b/docs/assets/readme-downloads.svg @@ -1,5 +1,5 @@ - - downloads: 90m + + downloads: 91m @@ -15,7 +15,7 @@ downloads downloads - 90m - 90m + 91m + 91m From ffd23fe25c7d1558aa12cd76c152a0343533381b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 4 Oct 2026 00:18:19 -0700 Subject: [PATCH 23/23] Avoid repeated runtime imports and recovery fixture seeding (#25155) --- docs/reference/ci-runner-efficiency.md | 43 ++++ ...le-state-recovery-crash-boundaries.test.ts | 101 ++++++++- .../orca-runtime-tail-wait-memo.test.ts | 209 +----------------- .../pty-transcript-prune-wait-cache.test.ts | 100 ++++++++- ...ned-tail-redraw-window.equivalence.test.ts | 6 +- src/main/runtime/terminal-projection.test.ts | 2 +- 6 files changed, 230 insertions(+), 231 deletions(-) diff --git a/docs/reference/ci-runner-efficiency.md b/docs/reference/ci-runner-efficiency.md index 19d928f3c82..1efd247e7bb 100644 --- a/docs/reference/ci-runner-efficiency.md +++ b/docs/reference/ci-runner-efficiency.md @@ -1941,6 +1941,49 @@ remained red. Its review recognized all five shards as a complete reference (10,608 files, 8,965,977 worker-ms). This was again a full fallback with `selectionEvaluated: false`, not evidence for enabling selected tests. +## October 4 runtime imports and recovery fixtures + +Three helper-only tests now import the existing terminal modules directly rather +than initializing the runtime service. Ten copied-loop cases never exercised +runtime memoization: they passed with its cache, timestamp update or prune +invalidation disabled. Two actual helper checks remain. The existing runtime +prune suite now exercises real leaf cache reuse, split prompt timestamps, +ordinary output, fresh prompts and detection after retained-history eviction. +Each of those three production faults fails a real runtime assertion. + +Recovery tests now seed three exact fixture variants once, after the seed child +has closed. Each crash still receives an independent byte-for-byte copy of the +entire database/WAL family and remapped paths. Buffer.equals retains exact byte +comparison without recursive matcher overhead. All 46 original crash boundaries +and retries remain. Four additional copy-isolation/WAL checks run, and teardown +requires that all seed bytes remain unchanged after the full suite. + +Three alternating one-worker hosted ARM pairs in +[37182181976](https://github.com/stablyai/orca/actions/runs/37182181976) +measured these complete invocations: + +| Cohort | Baseline seconds | Candidate seconds | Median saving | +| --- | --- | --- | --- | +| Three imports only, same 15 tests | 19.257 / 19.167 / 19.363 | 1.769 / 1.768 / 1.768 | 90.8% | +| Final four-file runtime cohort | 22.312 / 22.122 / 21.969 | 13.494 / 13.793 / 13.601 | 38.5% | +| Recovery crash boundaries | 24.082 / 24.075 / 24.814 | 8.061 / 8.105 / 9.074 | 66.3% | + +The final runtime cohort has seven real cases versus 16 including the copied +loops; its new runtime case is included in candidate timing. Recovery has 50 +passes versus the original 46. Hosted Node typecheck passed. Recovery faults for +last-byte database/WAL corruption, shared database paths, missing WAL copies and +accepted/unaccepted seed collision failed the intended assertions. These are +focused workload savings, not measured whole-shard or queue-delay gains. + +An independent local cache screen left both caches disabled. Across 14 unchanged +files and 92 cases, a warm Vitest transform cache reduced median invocation time +3.090 to 1.948 seconds, excluding archive costs; its cold arm increased time to +3.281 seconds. Node compilation caching showed no gain. Controls reproduced stale +transforms after TypeScript configuration or plugin-option changes, so persisted +reuse requires a complete transform-input stamp and hosted net-cost evidence. +A separate 130,000-pane leaf-collection optimization was restored: its complete +migration-file timing stayed within noise. The regression fixture remains. + ## October 3 removal fixture cleanup ordering [37105566358](https://github.com/stablyai/orca/actions/runs/37105566358) diff --git a/src/main/persistence/profile-state/profile-state-recovery-crash-boundaries.test.ts b/src/main/persistence/profile-state/profile-state-recovery-crash-boundaries.test.ts index 75e498b8d70..857e4f1c7ef 100644 --- a/src/main/persistence/profile-state/profile-state-recovery-crash-boundaries.test.ts +++ b/src/main/persistence/profile-state/profile-state-recovery-crash-boundaries.test.ts @@ -1,4 +1,5 @@ import { + cpSync, existsSync, mkdirSync, mkdtempSync, @@ -8,7 +9,7 @@ import { writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' -import { basename, dirname, join } from 'node:path' +import { basename, dirname, join, relative } from 'node:path' import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' import { setSecretStore } from '../../../shared/secret-store' import { profileStateStorage } from '../../orca-profiles/profile-project-state-file' @@ -103,13 +104,25 @@ afterEach(() => { rmSync(root, { recursive: true, force: true }) } }) -afterAll(() => rmSync(suiteRoot, { recursive: true, force: true })) +afterAll(() => { + try { + for (const seed of seededFixtures.values()) { + for (const [suffix, bytes] of seed.originalFamily) { + expect(readFileSync(`${seed.databasePath}${suffix}`).equals(bytes)).toBe(true) + } + expect(readFileSync(seed.backupPath).equals(seed.backupBytes)).toBe(true) + expect(readFileSync(seed.exportPath, 'utf8')).toBe(selectedJson) + } + } finally { + rmSync(suiteRoot, { recursive: true, force: true }) + } +}) type Fixture = RecoveryCrashOptions & { backupBytes: Buffer; originalFamily: Map } +const seededFixtures = new Map() -async function fixture(kind: 'json' | 'sqlite', accepted: boolean): Promise { - const root = mkdtempSync(join(suiteRoot, 'profile-')) - fixtureRoots.push(root) +async function seedFixture(kind: 'json' | 'sqlite', accepted: boolean): Promise { + const root = mkdtempSync(join(suiteRoot, 'seed-')) const directory = join(root, 'profiles', profileId) mkdirSync(directory, { recursive: true }) writeFileSync( @@ -173,6 +186,39 @@ async function fixture(kind: 'json' | 'sqlite', accepted: boolean): Promise [ + suffix, + readFileSync(`${options.databasePath}${suffix}`) + ]) + ) + return { ...options, backupBytes: readFileSync(options.backupPath), originalFamily } +} + +async function fixture(kind: 'json' | 'sqlite', accepted: boolean): Promise { + const key = `${kind}/${accepted}` + let seed = seededFixtures.get(key) + if (!seed) { + // The seed child's close event has fired before its WAL family is copied. + seed = await seedFixture(kind, accepted) + seededFixtures.set(key, seed) + } + return cloneFixture(seed) +} + function readSqlite(path: string): unknown { const opened = openProfileStateDatabaseReadOnly(path, profileId) try { @@ -192,10 +238,12 @@ function assertQuarantine(profile: Fixture): void { const directory = join(dirname(profile.databasePath), quarantine) // Check exact family bytes before opening the copied WAL snapshot. for (const [suffix, bytes] of profile.originalFamily) { - expect(readFileSync(join(directory, `profile-state.db${suffix}`))).toEqual(bytes) + expect(readFileSync(join(directory, `profile-state.db${suffix}`)).equals(bytes)).toBe(true) } expect(readFileSync(join(directory, basename(profile.exportPath)), 'utf8')).toBe(selectedJson) - expect(readFileSync(join(directory, basename(profile.backupPath)))).toEqual(profile.backupBytes) + expect( + readFileSync(join(directory, basename(profile.backupPath))).equals(profile.backupBytes) + ).toBe(true) expect(readSqlite(join(directory, 'profile-state.db'))).toEqual(oldState) } @@ -346,7 +394,7 @@ describe('SQLite recovery process death', () => { const profile = await fixture('sqlite', true) await killRecoveryAt(bundle, profile, stage(profile, boundary)) assertQuarantine(profile) - expect(readFileSync(profile.backupPath)).toEqual(profile.backupBytes) + expect(readFileSync(profile.backupPath).equals(profile.backupBytes)).toBe(true) const expected = [ 'marker-invalidated', 'selected-export', @@ -365,7 +413,7 @@ describe('SQLite recovery process death', () => { : 'refused' assertRestart(profile, expected) retry(profile) - expect(readFileSync(profile.backupPath)).toEqual(profile.backupBytes) + expect(readFileSync(profile.backupPath).equals(profile.backupBytes)).toBe(true) }) it.skipIf(process.platform === 'win32')( @@ -378,3 +426,38 @@ describe('SQLite recovery process death', () => { } ) }) + +describe('seeded recovery fixture copies', () => { + it.each([ + ['json', false], + ['json', true], + ['sqlite', true] + ] as const)( + 'isolates %s/accepted=%s through corruption and WAL reopen', + async (kind, accepted) => { + const first = await fixture(kind, accepted) + const second = await fixture(kind, accepted) + const seed = seededFixtures.get(`${kind}/${accepted}`) + if (!seed) { + throw new Error('Seed fixture was not retained') + } + expect(new Set([first.root, second.root, seed.root]).size).toBe(3) + for (const [suffix, bytes] of seed.originalFamily) { + expect(readFileSync(`${first.databasePath}${suffix}`).equals(bytes)).toBe(true) + writeFileSync(`${first.databasePath}${suffix}`, 'corrupted-copy') + expect(readFileSync(`${second.databasePath}${suffix}`).equals(bytes)).toBe(true) + expect(readFileSync(`${seed.databasePath}${suffix}`).equals(bytes)).toBe(true) + } + const third = await fixture(kind, accepted) + expect(readSqlite(third.databasePath)).toEqual(oldState) + expect(readFileSync(seed.backupPath).equals(seed.backupBytes)).toBe(true) + expect(readFileSync(seed.exportPath, 'utf8')).toBe(selectedJson) + } + ) + + it('rejects an incomplete copied WAL family before a recovery child starts', async () => { + const incomplete = await fixture('json', false) + rmSync(`${incomplete.databasePath}-wal`) + expect(() => cloneFixture(incomplete)).toThrow(/ENOENT/) + }) +}) diff --git a/src/main/runtime/orca-runtime-tail-wait-memo.test.ts b/src/main/runtime/orca-runtime-tail-wait-memo.test.ts index 5ce8d9710cd..4ac28674dd8 100644 --- a/src/main/runtime/orca-runtime-tail-wait-memo.test.ts +++ b/src/main/runtime/orca-runtime-tail-wait-memo.test.ts @@ -1,124 +1,7 @@ import { describe, expect, it, vi } from 'vitest' -import { - appendNormalizedToTailBuffer, - buildPreview, - computeTerminalTailWaitState, - tailGainedNewerBlockedReason, - type TerminalTailWaitState -} from './orca-runtime' +import { computeTerminalTailWaitState } from './terminal-wait-tail-state' -// These tests pin the onPtyData wait-detection memoization: caching the -// post-append wait state and reusing it as the next chunk's pre-append state -// must produce byte-for-byte the same waitBlockedAt stamping as recomputing the -// wait-state scan on both sides of every chunk (the pre-memoization behavior), -// while doing roughly half the state computations. - -type RedrawCursor = ReturnType['redrawCursor'] - -type TailSim = { - tailBuffer: string[] - tailPartialLine: string - tailRedrawCursor: RedrawCursor - preview: string - waitBlockedAt: number | null - tailWaitState?: TerminalTailWaitState -} - -function newSim(): TailSim { - return { - tailBuffer: [], - tailPartialLine: '', - tailRedrawCursor: null, - preview: '', - waitBlockedAt: null - } -} - -type Compute = typeof computeTerminalTailWaitState - -// Mirrors the memoized onPtyData tail loop: reuse the cached tail-derived state -// as the previous state; only recompute it on a preview-fallback (empty tail). -function stepMemoized(sim: TailSim, chunk: string, at: number, compute: Compute): void { - const previousWaitState = - sim.tailWaitState?.fromTail === true - ? sim.tailWaitState - : compute(sim.tailBuffer, sim.tailPartialLine, sim.preview) - const nextTail = appendNormalizedToTailBuffer( - sim.tailBuffer, - sim.tailPartialLine, - chunk, - sim.tailRedrawCursor - ) - const nextWaitState = compute(nextTail.lines, nextTail.partialLine, sim.preview) - if (tailGainedNewerBlockedReason(previousWaitState, nextWaitState, chunk)) { - sim.waitBlockedAt = at - } - sim.tailWaitState = nextWaitState - sim.tailBuffer = nextTail.lines - sim.tailPartialLine = nextTail.partialLine - sim.tailRedrawCursor = nextTail.redrawCursor - sim.preview = buildPreview(nextTail.lines, nextTail.partialLine) -} - -// Reference: the pre-memoization behavior — recompute the previous state fresh -// from the current tail on every chunk (no cache). -function stepReference(sim: TailSim, chunk: string, at: number, compute: Compute): void { - const previousWaitState = compute(sim.tailBuffer, sim.tailPartialLine, sim.preview) - const nextTail = appendNormalizedToTailBuffer( - sim.tailBuffer, - sim.tailPartialLine, - chunk, - sim.tailRedrawCursor - ) - const nextWaitState = compute(nextTail.lines, nextTail.partialLine, sim.preview) - if (tailGainedNewerBlockedReason(previousWaitState, nextWaitState, chunk)) { - sim.waitBlockedAt = at - } - sim.tailBuffer = nextTail.lines - sim.tailPartialLine = nextTail.partialLine - sim.tailRedrawCursor = nextTail.redrawCursor - sim.preview = buildPreview(nextTail.lines, nextTail.partialLine) -} - -function runBoth(chunks: string[]): { memoized: (number | null)[]; reference: (number | null)[] } { - const memoSim = newSim() - const refSim = newSim() - const memoized: (number | null)[] = [] - const reference: (number | null)[] = [] - chunks.forEach((chunk, index) => { - const at = index + 1 - stepMemoized(memoSim, chunk, at, computeTerminalTailWaitState) - stepReference(refSim, chunk, at, computeTerminalTailWaitState) - memoized.push(memoSim.waitBlockedAt) - reference.push(refSim.waitBlockedAt) - }) - return { memoized, reference } -} - -const SCENARIOS: Record = { - 'plain output never blocks': ['building...\n', 'compiled ok\n', 'watching for changes\n'], - 'blocked prompt in one chunk': ['Update available! Press Enter to continue.\n'], - 'blocked prompt split across chunks': ['Update available!\n', 'Press Enter to continue.\n'], - 'blocked then plain output stays blocked': [ - 'Update available! Press Enter to continue.\n', - 'still here\n', - 'more logs\n' - ], - 'partial lines without newline then completion': [ - 'Update ava', - 'ilable! Press Enter ', - 'to continue.\n' - ], - 'empty and whitespace chunks': ['', ' ', '\n', 'ok\n', ''], - 'ready header after stale blocked prompt': [ - 'Update available! Press Enter to continue.\n', - 'OpenAI Codex\n', - 'model: gpt\n', - 'directory: /repo\n' - ] -} - -describe('onPtyData tail wait memoization', () => { +describe('terminal tail wait state', () => { it('computeTerminalTailWaitState reports fromTail and blocked signals', () => { const empty = computeTerminalTailWaitState([], '', '') expect(empty.fromTail).toBe(false) @@ -151,92 +34,4 @@ describe('onPtyData tail wait memoization', () => { lastIndexOf.mockRestore() } }) - - for (const [name, chunks] of Object.entries(SCENARIOS)) { - it(`memoized stamping matches recompute reference: ${name}`, () => { - const { memoized, reference } = runBoth(chunks) - expect(memoized).toEqual(reference) - }) - } - - it('stays equivalent across tail eviction beyond the retained cap', () => { - const chunks: string[] = [] - for (let i = 0; i < 2600; i += 1) { - chunks.push(`line ${i} of streaming build output that keeps the tail busy\n`) - } - // Introduce a real blocked prompt well past the eviction boundary. - chunks.push('Update available! Press Enter to continue.\n') - chunks.push('trailing log after prompt\n') - const { memoized, reference } = runBoth(chunks) - expect(memoized).toEqual(reference) - // The prompt must actually be detected (guards against a vacuous match). - expect(memoized.at(-1)).not.toBeNull() - }) - - it('recomputes correctly after a transcript prune empties the tail (prune-then-resume)', () => { - // pruneDisconnectedPtyTranscript empties the tail AND clears tailWaitState; - // model that here and assert the memoized stamping still tracks a fresh - // recompute across the reset (a stale cache would desync the first resumed - // chunk, since the pre-prune tail held a blocked prompt). - const prune = (sim: TailSim): void => { - sim.tailBuffer = [] - sim.tailPartialLine = '' - sim.tailRedrawCursor = null - sim.preview = '' - sim.waitBlockedAt = null - sim.tailWaitState = undefined - } - const memoSim = newSim() - const refSim = newSim() - const memoOut: (number | null)[] = [] - const refOut: (number | null)[] = [] - let at = 0 - const feed = (chunk: string): void => { - at += 1 - stepMemoized(memoSim, chunk, at, computeTerminalTailWaitState) - stepReference(refSim, chunk, at, computeTerminalTailWaitState) - memoOut.push(memoSim.waitBlockedAt) - refOut.push(refSim.waitBlockedAt) - } - // Pre-prune: leave a stale blocked prompt in the tail. - ;['building\n', 'Update available! Press Enter to continue.\n', 'more log\n'].forEach(feed) - prune(memoSim) - prune(refSim) - // Resume: a fresh blocked prompt must be stamped, not masked by stale cache. - ;['fresh start\n', 'Update available! Press Enter to continue.\n', 'after\n'].forEach(feed) - - expect(memoOut).toEqual(refOut) - expect(memoSim.waitBlockedAt).not.toBeNull() - }) - - it('does roughly half the wait-state computations of the recompute reference', () => { - const chunks: string[] = [] - for (let i = 0; i < 500; i += 1) { - chunks.push(`streaming line ${i}\n`) - } - - let memoCalls = 0 - const countingMemo: Compute = (lines, partial, preview) => { - memoCalls += 1 - return computeTerminalTailWaitState(lines, partial, preview) - } - let refCalls = 0 - const countingRef: Compute = (lines, partial, preview) => { - refCalls += 1 - return computeTerminalTailWaitState(lines, partial, preview) - } - - const memoSim = newSim() - const refSim = newSim() - chunks.forEach((chunk, index) => { - stepMemoized(memoSim, chunk, index + 1, countingMemo) - stepReference(refSim, chunk, index + 1, countingRef) - }) - - // Reference recomputes both sides every chunk: 2 per chunk. - expect(refCalls).toBe(chunks.length * 2) - // Memoized reuses the cached previous state on every non-empty-tail chunk: - // 1 per chunk plus a single first-chunk cold miss. - expect(memoCalls).toBe(chunks.length + 1) - }) }) diff --git a/src/main/runtime/pty-transcript-prune-wait-cache.test.ts b/src/main/runtime/pty-transcript-prune-wait-cache.test.ts index 6c9082d2552..6a99c7b25ee 100644 --- a/src/main/runtime/pty-transcript-prune-wait-cache.test.ts +++ b/src/main/runtime/pty-transcript-prune-wait-cache.test.ts @@ -1,14 +1,9 @@ -/** - * Regression: pruneDisconnectedPtyTranscript empties a disconnected PTY's - * retained tail. The onPtyData wait-scan memoization caches the tail's wait - * state on the record (tailWaitState) and reuses it as the next chunk's - * "previous" state — so the prune MUST also clear that cache, or a record that - * resumes output after adoption/reattach would reuse a stale (pre-prune, - * possibly blocked) wait state and mis-stamp waitBlockedAt on its first chunk. - */ -import { describe, expect, it } from 'vitest' +// A disconnected transcript must not keep a wait-scan cache from before its history was pruned. +import { describe, expect, it, vi } from 'vitest' import { OrcaRuntimeService } from './orca-runtime' -import type { TerminalTailWaitState } from './orca-runtime' +import { MAX_TAIL_LINES } from './terminal-tail-limits' +import * as terminalWaitTailState from './terminal-wait-tail-state' +import type { TerminalTailWaitState } from './terminal-wait-tail-state' type PtyRecord = { connected: boolean @@ -20,9 +15,33 @@ type RuntimeInternals = { pruneDisconnectedPtyTranscript: (pty: PtyRecord) => void } +function onlyRuntimeLeaf(runtime: unknown) { + if (typeof runtime !== 'object' || runtime === null || !('leaves' in runtime)) { + throw new Error('Runtime has no leaves') + } + const leaves = runtime.leaves + if (!(leaves instanceof Map) || leaves.size !== 1) { + throw new Error('Expected exactly one runtime leaf') + } + const leaf: unknown = leaves.values().next().value + if ( + typeof leaf !== 'object' || + leaf === null || + !('tailBuffer' in leaf) || + !Array.isArray(leaf.tailBuffer) || + !('tailLinesTotal' in leaf) || + typeof leaf.tailLinesTotal !== 'number' || + !('waitBlockedAt' in leaf) + ) { + throw new Error('Runtime leaf has no terminal tail state') + } + return leaf +} + describe('pruneDisconnectedPtyTranscript clears the wait-scan cache', () => { it('empties the tail and drops tailWaitState so resume recomputes', () => { const runtime = new OrcaRuntimeService() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: both protected methods are declared on OrcaRuntimeService's inheritance chain. const internals = runtime as unknown as RuntimeInternals const pty = internals.recordPtyWorktree('pty-1', 'wt-1', { connected: true }) @@ -40,4 +59,65 @@ describe('pruneDisconnectedPtyTranscript clears the wait-scan cache', () => { expect(pty.tailBuffer).toEqual([]) expect(pty.tailWaitState).toBeUndefined() }) + + it('reuses the real leaf wait scan and stamps each newly arriving blocked prompt', async () => { + const runtime = new OrcaRuntimeService() + const ptyId = 'pty-memo' + const leafId = '11111111-1111-4111-8111-111111111111' + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { + tabs: [{ tabId: 'tab-1', worktreeId: 'wt-1', title: '', activeLeafId: leafId, layout: null }], + leaves: [ + { tabId: 'tab-1', worktreeId: 'wt-1', leafId, paneRuntimeId: 1, ptyId, paneTitle: null } + ] + }) + const leaf = onlyRuntimeLeaf(runtime) + // Different retained history exercises the leaf scan rather than the shared PTY-tail path. + leaf.tailBuffer = ['leaf-only history'] + leaf.tailLinesTotal = 1 + const scan = vi.spyOn(terminalWaitTailState, 'computeTerminalTailWaitState') + const leafScans = () => + scan.mock.calls.filter(([lines]) => lines.includes('leaf-only history')).length + try { + runtime.onPtyData(ptyId, 'first plain line\n', 1_000) + expect(leafScans()).toBe(2) + expect('tailWaitState' in leaf ? leaf.tailWaitState : undefined).toMatchObject({ + fromTail: true, + signal: null + }) + runtime.onPtyData(ptyId, 'second plain line\n', 2_000) + expect(leafScans()).toBe(3) + expect(leaf.waitBlockedAt).toBeNull() + runtime.onPtyData(ptyId, 'Update ava', 3_000) + expect(leafScans()).toBe(4) + expect(leaf.waitBlockedAt).toBeNull() + runtime.onPtyData(ptyId, 'ilable! Press Enter to continue.\n', 4_000) + expect(leafScans()).toBe(5) + expect(leaf.waitBlockedAt).toBe(4_000) + runtime.onPtyData(ptyId, 'ordinary log after the prompt\n', 5_000) + expect(leafScans()).toBe(6) + expect(leaf.waitBlockedAt).toBe(4_000) + runtime.onPtyData(ptyId, 'Update available! Press Enter to continue.\n', 6_000) + expect(leafScans()).toBe(7) + expect(leaf.waitBlockedAt).toBe(6_000) + runtime.onPtyData( + ptyId, + Array.from({ length: MAX_TAIL_LINES + 1 }, (_, index) => `streaming line ${index}\n`).join( + '' + ), + 7_000 + ) + expect(leaf.tailBuffer).toHaveLength(MAX_TAIL_LINES) + expect(leaf.tailBuffer).not.toContain('leaf-only history') + expect('tailWaitState' in leaf ? leaf.tailWaitState : undefined).toMatchObject({ + fromTail: true, + signal: null + }) + runtime.onPtyData(ptyId, 'Update available! Press Enter to continue.\n', 8_000) + expect(leaf.waitBlockedAt).toBe(8_000) + } finally { + scan.mockRestore() + await runtime.onPtyExit(ptyId, 0) + } + }) }) diff --git a/src/main/runtime/retained-tail-redraw-window.equivalence.test.ts b/src/main/runtime/retained-tail-redraw-window.equivalence.test.ts index 30b36483675..b11ce57dcfb 100644 --- a/src/main/runtime/retained-tail-redraw-window.equivalence.test.ts +++ b/src/main/runtime/retained-tail-redraw-window.equivalence.test.ts @@ -1,8 +1,6 @@ import { describe, expect, it } from 'vitest' -import { - appendNormalizedToTailBuffer, - appendNormalizedToMultilineTailBufferUnwindowed -} from './orca-runtime' +import { appendNormalizedToTailBuffer } from './terminal-tail-buffer' +import { appendNormalizedToMultilineTailBufferUnwindowed } from './terminal-tail-redraw-buffer' // Differential guard for the windowed redraw tail path: the public // appendNormalizedToTailBuffer routes vertical-control chunks through a diff --git a/src/main/runtime/terminal-projection.test.ts b/src/main/runtime/terminal-projection.test.ts index 7dff087e74c..c1634ddb19c 100644 --- a/src/main/runtime/terminal-projection.test.ts +++ b/src/main/runtime/terminal-projection.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from 'vitest' import type { TerminalCursorContext } from '../../shared/terminal-composer-draft' import type { HeadlessEmulator } from '../daemon/headless-emulator' -import { projectTerminalTailLines } from './orca-runtime' +import { projectTerminalTailLines } from './orca-runtime-terminal-projection' describe('projectTerminalTailLines', () => { it('does not splice a scrolled viewport into unrelated tail rows', () => {