Repository navigation
fix(terminal): serialize startup context with user paste - #11612
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const blocksUnsafeWindowsMultilinePaste = | ||
| shouldChunk && payload.lineCount > 1 && target.runtime.platform === 'win32' | ||
| const rejectReason = | ||
| payload.byteLength > maxBytes | ||
| ? 'payload-too-large' | ||
| : blocksUnsafeWindowsMultilinePaste | ||
| ? 'windows-multiline-paste-too-large' | ||
| : undefined |
There was a problem hiding this comment.
The blocking predicate target.runtime.platform === 'win32' matches every runtime whose resolved platform is win32, including ssh:windows-prod (the 'SSH Windows' entry in RUNTIME_MATRIX). The choosePasteMode → chunked path for SSH-into-Windows goes over the network rather than through a local ConPTY, so the ConPTY bracket-frame loss that motivates this guard may not apply in that case. This makes the block more conservative than necessary for remote Windows hosts. Worth a call-out to confirm the intent — if SSH Windows should be treated separately, an additional check like runtime.kind === 'local' or runtime.isWindowsConpty === true would narrow the condition to the concrete failure scenario.
| await installTerminalPtyWriteSpy(electronApp) | ||
| const ptyId = await waitForActivePanePtyId(orcaPage) | ||
| const payload = pastePayload(110) | ||
| const expectedText = payload.replace(/\r?\n/g, '\r') | ||
| // Why: assert on the normalized size so the payload keeps exercising the | ||
| // chunked (>64 KiB direct-max) lane even if planning ever measures | ||
| // post-normalization bytes. | ||
| expect(Buffer.byteLength(expectedText, 'utf8')).toBeGreaterThan(64 * 1024) |
There was a problem hiding this comment.
installTerminalPtyWriteSpy may miss writes that occur before the spy is installed
installTerminalPtyWriteSpy is called before waitForActivePanePtyId, but ptyId is obtained afterward and the spy starts recording from installation time. Any PTY write emitted between app startup and spy installation would be captured (before clearTerminalPtyWriteLog), but writes emitted between spy installation and the clear (e.g., from terminal initialisation) are also captured and then discarded. The ordering is sound. However, the spy checks entry.id === ptyId using a PTY ID that was acquired after installation, so there is a narrow window where the target terminal's PTY writes (e.g., shell startup echo) could arrive between installation and the clearTerminalPtyWriteLog call and would be correctly swept by the clear. The risk here is low but worth documenting: if clearTerminalPtyWriteLog is ever moved or removed, the assertion pasteWrites.toHaveLength(0) could produce false positives from shell startup writes on the target PTY.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Greptile SummaryThis PR fixes an input-ordering race where the agent startup draft and a concurrent user paste could interleave writes to the same PTY. The fix introduces
Confidence Score: 5/5Safe to merge; the transaction logic is correct, error paths release the lock in a finally block, and the race it fixes is well-tested at both unit and integration level. The core change — chaining PTY writes through a promise queue — is correctly implemented: the previous tail is captured before the new one is registered, the lock is always released in a finally block, and the map entry is cleaned up only by the last queued operation. No existing behavior is changed for non-contended paths. Files Needing Attention: terminal-paste-operation-order.test.ts — the new describe block does not call vi.resetModules() between tests, so a mid-test assertion failure could leave a stale map entry affecting subsequent tests.
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/terminal-pane/terminal-pty-input-transaction.ts | New module implementing a per-PTY promise-chaining serialization queue; logic is correct with proper error-safe cleanup in the finally block |
| src/renderer/src/lib/agent-draft-paste-content.ts | Wraps the public sendAgentDraftPasteContent in a transaction, extracting the prior implementation into sendAgentDraftPasteContentNow; the rename is clean and the logic is unchanged |
| src/renderer/src/components/terminal-pane/terminal-paste-executor.ts | Applies the same extract-and-wrap pattern for executeTerminalPastePlan; chunked writes now hold the PTY transaction lock for the duration of the operation |
| src/renderer/src/components/terminal-pane/terminal-paste-operation-order.test.ts | New test file covering uncontended start, startup-after-user-paste ordering, user-paste-after-startup ordering, cross-PTY isolation, and post-error recovery; does not reset module-level transactionTails state between tests |
| src/renderer/src/components/terminal-pane/pty-connection.test.ts | Renames and extends one existing test to send explicit user input before the composer prompt, verifying that the startup draft is ordered after both real user inputs |
Sequence Diagram
sequenceDiagram
participant User as User Input (onData)
participant Executor as executeTerminalPastePlan
participant Draft as sendAgentDraftPasteContent
participant TxQueue as runTerminalPtyInputTransaction (pty-1)
participant PTY as PTY Write
User->>Executor: paste triggered
Executor->>TxQueue: acquire lock (pty-1)
TxQueue-->>Executor: lock granted (no previous)
Executor->>PTY: BRACKETED_PASTE_START
Executor->>PTY: chunk1 (user-line-1)
Note over Executor: yieldToEventLoop()
Draft->>TxQueue: acquire lock (pty-1)
Note over TxQueue: previous = user-paste lock, queues behind it
Executor->>PTY: chunk2 (user-line-2)
Executor->>PTY: BRACKETED_PASTE_END
Executor->>TxQueue: release lock
TxQueue-->>Draft: lock granted
Draft->>PTY: BRACKETED_PASTE_START + content + BRACKETED_PASTE_END
Draft->>TxQueue: release lock
Reviews (8): Last reviewed commit: "perf(terminal): keep uncontended paste d..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/terminal-paste-runtime.ts (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication: identical platform-fallback chain in two branches.
transport?.getRemotePlatform?.() ?? remotePlatform ?? platformis now duplicated verbatim between theremote-runtimebranch (Line 38) and thesshbranch (Line 54). Consider extracting a small helper, e.g.resolveTransportPlatform(transport, remotePlatform, platform), to keep the two in sync going forward.♻️ Proposed helper extraction
+function resolveTransportPlatform( + transport: TerminalPasteRuntimeTransport | null | undefined, + remotePlatform: NodeJS.Platform | null | undefined, + platform: NodeJS.Platform +): NodeJS.Platform { + return transport?.getRemotePlatform?.() ?? remotePlatform ?? platform +} + export function resolveTerminalPasteRuntime({...}: ResolveTerminalPasteRuntimeArgs): TerminalPasteRuntime { ... if (isRemoteRuntimePastePtyId(ptyId)) { - const runtimePlatform = transport?.getRemotePlatform?.() ?? remotePlatform ?? platform + const runtimePlatform = resolveTransportPlatform(transport, remotePlatform, platform) ... } ... if (effectiveConnectionId) { - const sshPlatform = transport?.getRemotePlatform?.() ?? remotePlatform ?? platform + const sshPlatform = resolveTransportPlatform(transport, remotePlatform, platform) ... }Also applies to: 54-54
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 926627c8-e2ea-4155-931a-b514968fdd46
📒 Files selected for processing (7)
src/renderer/src/components/terminal-pane/terminal-paste-coordinator.test.tssrc/renderer/src/components/terminal-pane/terminal-paste-coordinator.tssrc/renderer/src/components/terminal-pane/terminal-paste-errors.tssrc/renderer/src/components/terminal-pane/terminal-paste-model.tssrc/renderer/src/components/terminal-pane/terminal-paste-runtime.test.tssrc/renderer/src/components/terminal-pane/terminal-paste-runtime.tstests/e2e/terminal-windows-codex-multiline-paste.spec.ts
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTerminal user input now updates PTY and pane input generations through unified handling. Clipboard and primary-selection paste flows record intent before retrieving or executing pasted text. Agent paste submission captures input checkpoints and stops when user input occurs before or during paste or before submission. Tests cover input generation, paste-intent ordering, startup-draft cancellation, split isolation, and interrupted chunked paste. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bad92b5 to
3d72d1e
Compare
OrcaWin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ad9aad3-24d6-44ef-8888-703f642654e6
📒 Files selected for processing (2)
src/renderer/src/components/dashboard-popout/AgentTerminalPreview.test.tsxsrc/renderer/src/components/dashboard-popout/preview-terminal-paste.ts
| it('protects a Windows draft from five-line preview paste submission', async () => { | ||
| platformState.value = 'win32' | ||
| const pastedText = ['a', 'b', 'c', 'd', 'e'].join('\r\n') | ||
| readClipboardText.mockResolvedValueOnce(pastedText) | ||
| const view = render(<AgentTerminalPreview ptyId="pty-1" />) | ||
| await waitFor(() => expect(terminalHarness.instances).toHaveLength(1)) | ||
| const terminal = terminalHarness.instances[0]! | ||
| const host = view.container.querySelector<HTMLElement>('.origin-bottom-left')! | ||
| const terminalInput = document.createElement('input') | ||
| host.appendChild(terminalInput) | ||
| terminalInput.focus() | ||
|
|
||
| act(() => emitAppMenuPaste!()) | ||
|
|
||
| const protectedPaste = `${BRACKETED_PASTE_START}a\rb\rc\rd\re${BRACKETED_PASTE_END}` | ||
| await waitFor(() => expect(input).toHaveBeenCalledWith('pty-1', protectedPaste)) | ||
| expect(terminal.paste).not.toHaveBeenCalled() | ||
| expect(terminal.input).toHaveBeenCalledWith(protectedPaste) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the protected frame is the only delivery.
toHaveBeenCalledWith passes even if the handler also emits an unprotected newline or duplicate frame, so this regression test would not catch the original submission bug. Assert exactly one PTY and terminal-input call.
Suggested test tightening
- await waitFor(() => expect(input).toHaveBeenCalledWith('pty-1', protectedPaste))
+ await waitFor(() => {
+ expect(input).toHaveBeenCalledTimes(1)
+ expect(input).toHaveBeenCalledWith('pty-1', protectedPaste)
+ })
expect(terminal.paste).not.toHaveBeenCalled()
+ expect(terminal.input).toHaveBeenCalledTimes(1)
expect(terminal.input).toHaveBeenCalledWith(protectedPaste)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('protects a Windows draft from five-line preview paste submission', async () => { | |
| platformState.value = 'win32' | |
| const pastedText = ['a', 'b', 'c', 'd', 'e'].join('\r\n') | |
| readClipboardText.mockResolvedValueOnce(pastedText) | |
| const view = render(<AgentTerminalPreview ptyId="pty-1" />) | |
| await waitFor(() => expect(terminalHarness.instances).toHaveLength(1)) | |
| const terminal = terminalHarness.instances[0]! | |
| const host = view.container.querySelector<HTMLElement>('.origin-bottom-left')! | |
| const terminalInput = document.createElement('input') | |
| host.appendChild(terminalInput) | |
| terminalInput.focus() | |
| act(() => emitAppMenuPaste!()) | |
| const protectedPaste = `${BRACKETED_PASTE_START}a\rb\rc\rd\re${BRACKETED_PASTE_END}` | |
| await waitFor(() => expect(input).toHaveBeenCalledWith('pty-1', protectedPaste)) | |
| expect(terminal.paste).not.toHaveBeenCalled() | |
| expect(terminal.input).toHaveBeenCalledWith(protectedPaste) | |
| }) | |
| it('protects a Windows draft from five-line preview paste submission', async () => { | |
| platformState.value = 'win32' | |
| const pastedText = ['a', 'b', 'c', 'd', 'e'].join('\r\n') | |
| readClipboardText.mockResolvedValueOnce(pastedText) | |
| const view = render(<AgentTerminalPreview ptyId="pty-1" />) | |
| await waitFor(() => expect(terminalHarness.instances).toHaveLength(1)) | |
| const terminal = terminalHarness.instances[0]! | |
| const host = view.container.querySelector<HTMLElement>('.origin-bottom-left')! | |
| const terminalInput = document.createElement('input') | |
| host.appendChild(terminalInput) | |
| terminalInput.focus() | |
| act(() => emitAppMenuPaste!()) | |
| const protectedPaste = `${BRACKETED_PASTE_START}a\rb\rc\rd\re${BRACKETED_PASTE_END}` | |
| await waitFor(() => { | |
| expect(input).toHaveBeenCalledTimes(1) | |
| expect(input).toHaveBeenCalledWith('pty-1', protectedPaste) | |
| }) | |
| expect(terminal.paste).not.toHaveBeenCalled() | |
| expect(terminal.input).toHaveBeenCalledTimes(1) | |
| expect(terminal.input).toHaveBeenCalledWith(protectedPaste) | |
| }) |
3d72d1e to
affbf33
Compare
OrcaWin
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/terminal-pane/TerminalPane.tsx (1)
2672-2749: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrimary-selection middle-click paste still records input after the async paste, not before — reintroduces the race this PR fixes elsewhere.
Every other paste entry point in this PR (
pasteFromClipboard,onAppMenuPaste, andpasteResolvedPaneinuse-terminal-pane-context-menu.ts) now callsrecordTerminalUserInputForLeaf/onPasteIntentsynchronously before the async clipboard/paste work starts, sodidTerminalUserInputChangeinagent-paste-draft.tscan observe the takeover in time.handlePrimarySelectionMiddleMouseDownwas not updated: it only callsrecordTerminalUserInputForLeaf(tabId, clickedPane.leafId)at Line 2745, afterreadPrimarySelectionText()→planTerminalPasteWithYield→executeTerminalPastePlanhave all resolved. A middle-click paste landing during that async window (while a background agent draft-paste is checking for user takeover) would go undetected, letting generated linked-work-item content and the user's real paste mix/submit together — the exact bug this PR targets, just for this one entry point.🐛 Proposed fix: record paste intent before the async primary-selection read
armPrimarySelectionNativePasteSuppression() clickedPane.terminal.focus() + // Why: mirror onPasteIntent — record real user input before the async + // read so a concurrent agent draft-paste can detect the takeover. + recordTerminalUserInputForLeaf(tabId, clickedPane.leafId) void readPrimarySelectionText().then(async (text) => { if (!text) { return } @@ if (execution.status !== 'pasted') { setTerminalError(formatTerminalPasteExecutionError(execution.reason)) return } - recordTerminalUserInputForLeaf(tabId, clickedPane.leafId) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e39589c9-de1e-437f-b2f3-cc0054f90c78
📒 Files selected for processing (9)
src/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/terminal-clipboard-paste.test.tssrc/renderer/src/components/terminal-pane/terminal-clipboard-paste.tssrc/renderer/src/components/terminal-pane/terminal-input-activity.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-context-menu.tssrc/renderer/src/lib/agent-paste-draft.test.tssrc/renderer/src/lib/agent-paste-draft.ts
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01486758-9751-4130-a0c9-0ac6c4a9305b
📒 Files selected for processing (8)
src/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/terminal-input-activity.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-context-menu.tssrc/renderer/src/lib/agent-draft-paste-content.tssrc/renderer/src/lib/agent-paste-draft.test.tssrc/renderer/src/lib/agent-paste-draft.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/components/terminal-pane/use-terminal-pane-context-menu.ts
- src/renderer/src/components/terminal-pane/TerminalPane.tsx
- src/renderer/src/lib/agent-paste-draft.ts
| const recordRealTerminalUserInput = (): void => { | ||
| markTerminalUserInputForPtyId(transport.getPtyId(), cacheKey) | ||
| recordTerminalInputForHibernation() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate subscribeToTerminalUserInput to confirm it can return null.
rg -nP -C 8 '\bexport function subscribeToTerminalUserInput\b' --type=tsRepository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'src/renderer/src/**' | rg 'pty-connection|terminal|hibernation' || true
echo "== pty-connection line count =="
wc -l src/renderer/src/components/terminal-pane/pty-connection.ts || true
echo "== relevant symbols in pty-connection =="
rg -n -C 4 '\b(subscribeToTerminalUserInput|recordTerminalInputForHibernation|markTerminalUserInputForPtyId|user-input|userInputActivityDisposable)\b' src/renderer/src/components/terminal-pane/pty-connection.ts || true
echo "== focused lines 3580-3675 =="
sed -n '3580,3675p' src/renderer/src/components/terminal-pane/pty-connection.ts | cat -nRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate symbols anywhere in TS/TSX =="
rg -n -C 4 '\bsubscribeToTerminalUserInput\b|\brecordTerminalInputForHibernationFallback\b|\brecordTerminalInputForHibernation\b|\bmarkTerminalUserInputForPtyId\b|\buserInputActivityDisposable\b' src/renderer/src --glob '*.ts' --glob '*.tsx' || true
echo "== pty-connection focused lines =="
sed -n '3580,3675p' src/renderer/src/components/terminal-pane/pty-connection.ts | cat -n
echo "== nearby definitions/usages =="
rg -n -C 6 '\brecordAcceptedTerminalInputSent\b|\bmarkAcceptedTerminalInputSent\b|\brecordTerminalInputForHibernationFallback\b|\bsubscribeToTerminalUserInput\b' src/renderer/src/components/terminal src/renderer/src/lib src/renderer/src/hooks --glob '*.ts' --glob '*.tsx' || trueRepository: stablyai/orca
Length of output: 29865
Call markTerminalUserInputForPtyId in the onData fallback path.
When subscribeToTerminalUserInput() returns null, the listener is installed on onData but markTerminalUserInputForPtyId() is not called. This disables the user-input checkpoint for that pane, even though hibernation activity still records the onData input. Add a sibling fallback that also calls markTerminalUserInputForPtyId() before recordTerminalInputForHibernation().
Superseded validationThe earlier version of this comment exercised explicit The PR has been reworked around the reproduced nested bracketed-paste race. Current Windows Electron/ConPTY validation and screenshots are in the updated PR description:
|
OrcaWin
changed the title
) * fix(agents): prevent startup paste from submitting user input * fix(terminal): cancel startup submit on middle-click paste * fix(terminal): cancel startup context on first user input * fix(terminal): record activity only for accepted paste * fix(terminal): serialize bracketed paste operations * Revert "fix(terminal): cancel startup context on first user input" This reverts commit 5459ce7. * Revert "fix(agents): prevent startup paste from submitting user input" This reverts commit affbf33. * Revert "fix(terminal): cancel startup submit on middle-click paste" This reverts commit 6775751. * test(terminal): cover startup and user paste ordering * perf(terminal): keep uncontended paste delivery synchronous
Summary
Root cause
The normal linked-worktree path defaults to
promptDelivery: 'draft'. It does not send Enter after injecting the context.The confirmed transport failure was nested bracketed-paste framing. A Windows multiline paste over 64 KiB is written as one bracketed frame across acknowledged 16 KiB chunks, with event-loop yields between chunks. Startup-owned linked context is also sent as a bracketed paste. Both producers shared the mounted PTY transport, but there was no whole-operation owner.
That allowed this byte order:
The generated
ENDcould close Codex's paste mode early. Carriage returns in the remaining user chunks were then outside the intended user paste frame and could be interpreted as submit input.The fix gives each PTY a FIFO paste transaction covering the complete
START ... ENDoperation. If the user paste begins first, linked context waits and is injected afterward. If generated context begins first, the user paste waits. Typing and both payloads are preserved; normal draft launch still sends no Enter.The explicit
submit-after-readyflow is intentionally unchanged and is not part of this incident.Reproduction
The regression test deterministically exercises both legal races:
Before the fix, the user-first case produced a nested generated frame and exposed later carriage returns outside the intended paste frame. After the fix, each frame completes before the other begins.
The exact real-Codex visual outcome—part of the text submitting while the remainder appears as
[Pasted n characters]—was not reproduced end to end on the broken revision. This PR fixes the underlying overlapping-frame race that can produce that class of split-paste behavior. If the same UI symptom recurs after this change, it has another input source and needs a separate trace.Electron validation
Validated on Windows Electron/ConPTY at PR head
8c1946e2da633fe58c05ad7c24f8a46631298fddthrough the production startup-owned draft path.The validation held the acknowledged user paste after its opening marker, allowed the fake Codex readiness signal to fire, then released it. The terminal verified:
The validation harness and screenshots are not part of the PR diff.
Testing
git diff --checkPerformance and compatibility
The transaction registry stores only active/queued operations and deletes a PTY entry when its tail settles. Uncontended operations start synchronously, preserving the existing input latency. Different PTYs do not block each other. The shared renderer path covers local, WSL, SSH, and remote-runtime terminals without changing Git, provider, worktree, or folder-workspace behavior.