Repository navigation
fix(terminal): serialize agent-prompt submits per PTY so overlapping sends stop gluing #14980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
985c26d
a6c4857
288d1b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { mergeRejectedDraftInto } from './mobile-native-chat-rejected-draft-merge' | ||
|
|
||
| describe('mergeRejectedDraftInto', () => { | ||
| it('restores a rejected send into an empty composer verbatim', () => { | ||
| expect(mergeRejectedDraftInto({ a: '' }, 'a', 'ping')).toEqual({ a: 'ping' }) | ||
| }) | ||
|
|
||
| it('restores into a composer key that does not exist yet', () => { | ||
| expect(mergeRejectedDraftInto({}, 'a', 'ping')).toEqual({ a: 'ping' }) | ||
| }) | ||
|
|
||
| it('keeps both texts when the user typed while the rejection was in flight', () => { | ||
| // The rejected send has no other surviving copy; skipping it loses the message. | ||
| expect(mergeRejectedDraftInto({ a: 'newer edit' }, 'a', 'ping')).toEqual({ | ||
| a: 'ping\nnewer edit' | ||
| }) | ||
| }) | ||
|
|
||
| it('does not duplicate a rejected send the composer already holds', () => { | ||
| const drafts = { a: 'ping' } | ||
| expect(mergeRejectedDraftInto(drafts, 'a', 'ping')).toBe(drafts) | ||
| }) | ||
|
|
||
| it('preserves whitespace on both sides of the merge', () => { | ||
| // Trailing whitespace is load-bearing: the host writes it onto the agent's | ||
| // input line verbatim. | ||
| expect(mergeRejectedDraftInto({ a: ' spaced ' }, 'a', 'ping ')).toEqual({ | ||
| a: 'ping \n spaced ' | ||
| }) | ||
| }) | ||
|
|
||
| it('leaves other tabs untouched', () => { | ||
| expect(mergeRejectedDraftInto({ a: '', b: 'other' }, 'a', 'ping')).toEqual({ | ||
| a: 'ping', | ||
| b: 'other' | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /** | ||
| * Restoring the text of a send the host definitely rejected. | ||
| * | ||
| * Merge, never skip. The composer is cleared at send time, and a rejected send | ||
| * never reaches `acceptSend`, so the rejected text has no other surviving copy — | ||
| * declining to restore it because the user has since typed again loses the | ||
| * message outright. Over relay/SSH that window is seconds, not milliseconds. | ||
| * | ||
| * Ordering is by authorship: the rejected text was composed before whatever was | ||
| * typed after it, so it goes first and the newer text follows on its own line. | ||
| */ | ||
| export function mergeRejectedDraftInto( | ||
| drafts: Record<string, string>, | ||
| draftKey: string, | ||
| rejected: string | ||
| ): Record<string, string> { | ||
| const current = drafts[draftKey] ?? '' | ||
| // An identical composer already holds the text; appending would duplicate it. | ||
| if (current === rejected) { | ||
| return drafts | ||
| } | ||
| return { ...drafts, [draftKey]: current === '' ? rejected : `${rejected}\n${current}` } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,7 @@ import { buildOrchestrationTaskDisplayMetadata } from '../../shared/orchestratio | |
| import { | ||
| isTerminalInputTooLargeWithYield, | ||
| TERMINAL_INPUT_TOO_LARGE_ERROR, | ||
| TERMINAL_SEND_SUBMIT_DELAY_MS, | ||
| iterateTerminalInputChunks | ||
| } from '../../shared/terminal-input' | ||
| import { | ||
|
|
@@ -18346,7 +18347,59 @@ export class OrcaRuntimeService { | |
| return bestStatus ? { status: bestStatus, updatedAt: bestUpdatedAt } : null | ||
| } | ||
|
|
||
| private async writeTerminalAction( | ||
| // Why: a submit writes its body, waits a frame for the TUI to render it, then | ||
| // writes Enter. That wait is not atomic, so two overlapping sends to one PTY | ||
| // interleave across it and their bytes land on ONE agent input line — Orca | ||
| // manufacturing the merged transcript row the pending-send matchers then have | ||
| // to recognize. Serialize submits per PTY. Same shape as `enqueueLayout` | ||
| // without the coalescing: every prompt must be delivered, none may be dropped. | ||
| private terminalSubmitQueues = new Map<string, Promise<void>>() | ||
|
|
||
| private enqueueTerminalSubmit(ptyId: string, run: () => Promise<void>): Promise<void> { | ||
| const previous = this.terminalSubmitQueues.get(ptyId) ?? Promise.resolve() | ||
| const slot = previous.then(run) | ||
| // The stored tail absorbs the rejection so one failed send cannot jam the | ||
| // queue; the caller still sees its own error via `slot`. | ||
| const tail = slot.then( | ||
| () => undefined, | ||
| () => undefined | ||
| ) | ||
| this.terminalSubmitQueues.set(ptyId, tail) | ||
| void tail.then(() => { | ||
| // Drop the entry once drained so the map does not grow across short-lived PTYs. | ||
| if (this.terminalSubmitQueues.get(ptyId) === tail) { | ||
| this.terminalSubmitQueues.delete(ptyId) | ||
| } | ||
| }) | ||
| return slot | ||
| } | ||
|
|
||
| private writeTerminalAction( | ||
| ptyId: string, | ||
| action: { text?: string; enter?: boolean; interrupt?: boolean }, | ||
| payload: string, | ||
| options: { | ||
| beforeWrite?: (ptyId: string) => void | Promise<void> | ||
| reserveWrite?: (ptyId: string) => void | ||
| afterWrite?: (ptyId: string) => void | Promise<void> | ||
| suffixFailureError?: string | ||
| } = {} | ||
| ): Promise<void> { | ||
| const isSubmit = | ||
| typeof action.text === 'string' && action.text.length > 0 && Boolean(action.enter) | ||
| // Why: only a body→delay→suffix sequence owns a gap to interleave across. | ||
| // Single-write actions — a bare interrupt, a bare Enter, keystrokes with no | ||
| // submit — stay unqueued so Ctrl+C and live typing are never held behind a | ||
| // queued send. | ||
| if (!isSubmit) { | ||
| return this.runTerminalAction(ptyId, action, payload, options) | ||
| } | ||
| return this.enqueueTerminalSubmit(ptyId, () => | ||
| this.runTerminalAction(ptyId, action, payload, options) | ||
| ) | ||
| } | ||
|
|
||
|
Comment on lines
+18377
to
+18401
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- relevant runtime implementation ---'
sed -n '18330,18445p' src/main/runtime/orca-runtime.ts
printf '%s\n' '--- action type and terminal send callers ---'
rg -n -C 8 'RuntimeTerminalSend|sendTerminal|writeTerminalAction|writeTerminalAgentPrompt|enqueueTerminalSubmit' src/main/runtime/orca-runtime.ts src -g '*.ts' -g '*.tsx'Repository: stablyai/orca Length of output: 50370 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- runtime implementation ---'
sed -n '18350,18440p' src/main/runtime/orca-runtime.ts
printf '%s\n' '--- focused declarations and call sites ---'
rg -n -C 5 --glob '*.ts' --glob '*.tsx' \
'type RuntimeTerminalSend|interface RuntimeTerminalSend|RuntimeTerminalSend|sendTerminal\s*\(|writeTerminalAction\s*\(' \
src/main/runtime src/shared src/renderer | head -n 500Repository: stablyai/orca Length of output: 43000 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- terminal.send input schema and flag handling ---'
sed -n '280,365p' src/main/runtime/rpc/methods/terminal.ts
sed -n '1340,1405p' src/main/runtime/rpc/methods/terminal.ts
printf '%s\n' '--- action construction and payload semantics ---'
rg -n -C 8 'buildSendPayload|terminal\.send|interrupt.*enter|enter.*interrupt' src/main/runtime src/shared docs README.md -g '*.ts' -g '*.tsx' -g '*.md' 2>/dev/null | head -n 400Repository: stablyai/orca Length of output: 35670 Serialize text-plus-interrupt terminal actions. When |
||
| private async runTerminalAction( | ||
| ptyId: string, | ||
| action: { text?: string; enter?: boolean; interrupt?: boolean }, | ||
| payload: string, | ||
|
|
@@ -18367,7 +18420,7 @@ export class OrcaRuntimeService { | |
| if (hasSuffix) { | ||
| const suffix = (action.enter ? '\r' : '') + (action.interrupt ? '\x03' : '') | ||
| if (hasText) { | ||
| await new Promise((resolve) => setTimeout(resolve, 500)) | ||
| await new Promise((resolve) => setTimeout(resolve, TERMINAL_SEND_SUBMIT_DELAY_MS)) | ||
| } | ||
| try { | ||
| await options.beforeWrite?.(ptyId) | ||
|
|
@@ -18424,7 +18477,21 @@ export class OrcaRuntimeService { | |
| } | ||
| } | ||
|
|
||
| private async writeTerminalAgentPrompt( | ||
| private writeTerminalAgentPrompt( | ||
| ptyId: string, | ||
| pastePayload: string, | ||
| options: { | ||
| beforeWrite?: (ptyId: string) => void | Promise<void> | ||
| suffixFailureError?: string | ||
| } = {} | ||
| ): Promise<void> { | ||
| // Always a submit: paste body, render gate (or fixed delay), then Enter. | ||
| return this.enqueueTerminalSubmit(ptyId, () => | ||
| this.writeAgentPromptFrame(ptyId, pastePayload, options) | ||
| ) | ||
| } | ||
|
|
||
| private async writeAgentPromptFrame( | ||
| ptyId: string, | ||
| pastePayload: string, | ||
| options: { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the
restoreRejectedDraftcontract comment.Line 66 says restoration is skipped when newer edits exist. This callback now merges the rejected text before those edits. Update the comment to state the new behavior.
Proposed change