fix(pty): keep a CPR reply from overtaking a deferred colour reply

A background-colour probe writes `OSC 11 ;? ST` then `CSI 6n` and reads
exactly one response, using the CPR as its sentinel: a non-OSC first
response means "unsupported" and it stops draining. #13309 routed live
cooked-echo-risk replies through the ECHO-probe deferral while CPR kept
the immediate path, so the CPR overtook the colour reply, the prober gave
up, and the stray `ESC ]` was left in the tty for the next program —
`gh auth login` died on it with an escape-sequence error.

Queue a reply that needs no echo containment behind ones that do, FIFO,
and only while something is actually deferred, so latency-critical
replies stay immediate on every other path. Windows is unaffected: only
posix-pty defers, so the queue is always empty there.

Also: an in-flight echo probe is already the write continuation, so
re-arming the timer for a queued reply would fork a second stty and throw
away the first verdict; and teardown now hands queued uncontained writes
to the pty best-effort instead of dropping bytes the caller was told were
sent.

Known scope limit, pinned by tests and tracked for follow-up: the
guarantee is FIFO among recognised query replies, not over every byte —
a reply coalesced with a keystroke, and ordinary typed input, still
bypass the queue. Both were unordered before this change too.
This commit is contained in:
Neil
2026-08-19 20:45:25 -07:00
parent a76a95d111
commit 57bd1dff77
8 changed files with 368 additions and 39 deletions
+2 -5
View File
@@ -12,7 +12,7 @@ import type { SessionOptions } from './session-options'
import type { TuiAgent } from '../../shared/tui-agent'
import { randomUUID } from 'node:crypto'
import { PtyStartupIngress } from '../../shared/pty-startup-ingress'
import { extractOnlyCookedEchoSafeQueryReplies } from '../../shared/terminal-query-reply'
import { takeLiveQueryReply } from '../../shared/terminal-query-reply'
import type {
SessionState,
ShellReadyState,
@@ -132,10 +132,7 @@ export class Session {
}
// Daemon POSIX PTYs need the local provider's cooked-echo containment (#13137).
if (
extractOnlyCookedEchoSafeQueryReplies(data) &&
this.startupIngress.answerLiveQueryReply(data)
) {
if (takeLiveQueryReply(this.startupIngress, data)) {
return
}
+5 -7
View File
@@ -72,7 +72,7 @@ import { ORCA_HERMES_STARTUP_QUERY_ENV } from '../../shared/hermes-startup-query
import { PhysicalExitTracker } from '../../shared/physical-exit-tracker'
import { mergeGitConfigEnvProtocol } from '../../shared/git-credential-prompt-env'
import { PtyStartupIngress, type PtyIngressEmission } from '../../shared/pty-startup-ingress'
import { extractOnlyCookedEchoSafeQueryReplies } from '../../shared/terminal-query-reply'
import { takeLiveQueryReply } from '../../shared/terminal-query-reply'
import { resolvePtyOwnerBackend } from '../../shared/pty-owner-backend'
import { signalPosixPtyForegroundGroup } from '../pty/posix-pty-foreground-group'
import { readPtsName } from '../pty/node-pty-pts-name'
@@ -1192,12 +1192,10 @@ export class LocalPtyProvider implements IPtyProvider {
return ptyProcesses.has(id)
}
write(id: string, data: string): boolean {
// Cooked PTYs echo private DSR/OSC replies; CPR/DA remain immediate (#13137, #7329).
if (extractOnlyCookedEchoSafeQueryReplies(data)) {
const ingress = startupIngressByPty.get(id)
if (ingress?.answerLiveQueryReply(data)) {
return true
}
// Cooked PTYs echo private DSR/OSC replies; CPR/DA stay immediate unless one of
// those is still deferred, which they must not overtake (#13137, #7329).
if (takeLiveQueryReply(startupIngressByPty.get(id), data)) {
return true
}
const proc = ptyProcesses.get(id)
if (!proc) {
+2 -5
View File
@@ -58,7 +58,7 @@ import {
parsePtyStartupIngressIntent,
type PtyIngressEmission
} from '../shared/pty-startup-ingress'
import { extractOnlyCookedEchoSafeQueryReplies } from '../shared/terminal-query-reply'
import { takeLiveQueryReply } from '../shared/terminal-query-reply'
import { resolvePtyOwnerBackend, type PtyOwnerBackend } from '../shared/pty-owner-backend'
import { RecentPtyOutputBuffer } from '../main/runtime/recent-pty-output-buffer'
import { expandWindowsPathEnvironmentVariables } from '../shared/windows-environment-expansion'
@@ -1881,10 +1881,7 @@ export class PtyHandler {
this.lastInputAtByPty.set(id, performance.now())
this.interactiveOutputCharsByPty.set(id, 0)
// Relay PTYs need the local provider's cooked-echo containment (#13137).
if (
extractOnlyCookedEchoSafeQueryReplies(data) &&
managed.startupIngress?.answerLiveQueryReply(data)
) {
if (takeLiveQueryReply(managed.startupIngress, data)) {
return
}
managed.pty.write(data)
@@ -15,6 +15,8 @@ const COLOR_SCHEME_REPLY = mode2031SequenceFor('dark')
// CSI has only a caret echo; OSC also has readline's rewritten echo.
const POSIX_CSI_COOKED_ECHO = (reply: string): string => reply.replaceAll('\x1b', '^[')
const OSC_COLOR_REPLY = '\x1b]11;rgb:00/00/00\x07'
// Latency-critical, so it stays off the contained path — but must not overtake one.
const CPR_REPLY = '\x1b[6;1R'
const POSIX_OSC_COOKED_ECHOES = [
(reply: string): string => reply.replaceAll('\x1b', '^['),
(reply: string): string => reply.replaceAll('\x1b]', '\x07').replaceAll('\x1b\\', '')
@@ -162,6 +164,164 @@ describe('PtyStartupIngress live query replies (#13137)', () => {
ingress.drainAndClose()
})
// termenv writes `OSC 11 ;? ST` then `CSI 6n` and stops reading at the CPR, so a
// CPR that overtakes the still-deferred color reply strands it for the next program
// (`gh auth login` -> "unexpected escape sequence from terminal: ['\x1b' ']']").
it('holds a CPR reply behind a still-deferred color reply', async () => {
vi.useFakeTimers()
const writes: string[] = []
const probe = scriptedEchoProbe('echoing', 'quiet')
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: probe,
write: (data) => writes.push(data),
onEmission: () => {}
})
expect(ingress.answerLiveQueryReply(OSC_COLOR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(0)
expect(writes).toEqual([])
expect(ingress.writeQueryReplyInOrder(CPR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(20)
expect(writes).toEqual([OSC_COLOR_REPLY, CPR_REPLY])
ingress.drainAndClose()
})
it('leaves a CPR reply on the caller immediate path when nothing is deferred', async () => {
vi.useFakeTimers()
const writes: string[] = []
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: scriptedEchoProbe('quiet'),
write: (data) => writes.push(data),
onEmission: () => {}
})
expect(ingress.writeQueryReplyInOrder(CPR_REPLY)).toBe(false)
expect(ingress.answerLiveQueryReply(OSC_COLOR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(0)
expect(writes).toEqual([OSC_COLOR_REPLY])
expect(ingress.writeQueryReplyInOrder(CPR_REPLY)).toBe(false)
ingress.drainAndClose()
})
it('does not project an ordered CPR reply as an echo to swallow', async () => {
vi.useFakeTimers()
const emissions: PtyIngressEmission[] = []
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
// Never settles, so the budget-expiry flush runs with kernelEchoImpossible
// false — the only verdict under which the caret shape IS armed, so this is
// what discriminates the uncontained branch from the contained one.
echoProbe: () => new Promise(() => {}),
write: () => {},
onEmission: (emission) => emissions.push(emission)
})
expect(ingress.answerLiveQueryReply(OSC_COLOR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(0)
expect(ingress.writeQueryReplyInOrder(CPR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(200)
// The CPR rode the queue for order only, so its caret shape is ordinary output.
ingress.accept(POSIX_CSI_COOKED_ECHO(CPR_REPLY))
expect(visible(emissions)).toBe(POSIX_CSI_COOKED_ECHO(CPR_REPLY))
ingress.drainAndClose()
})
// An ordered reply must not restart the ECHO probe: attemptPendingWrites nulls
// writeTimer on entry, so re-arming forks a second stty and discards the in-flight
// verdict, delaying the contained reply this queue exists to deliver first.
it('queues ordered replies without spawning extra echo probes', async () => {
vi.useFakeTimers()
const counts: number[] = []
for (const orderedCount of [0, 8]) {
const writes: string[] = []
let calls = 0
// Never settles, so the probe stays in flight across the whole queueing window.
const probe = (): Promise<never> => {
calls += 1
return new Promise(() => {})
}
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: probe,
write: (data) => writes.push(data),
onEmission: () => {}
})
expect(ingress.answerLiveQueryReply(OSC_COLOR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(0)
for (let i = 0; i < orderedCount; i += 1) {
ingress.writeQueryReplyInOrder(CPR_REPLY)
}
await vi.advanceTimersByTimeAsync(200)
expect(writes[0]).toBe(OSC_COLOR_REPLY)
expect(writes).toHaveLength(1 + orderedCount)
counts.push(calls)
ingress.drainAndClose()
}
expect(counts).toEqual([1, 1])
})
it('keeps holding a contained reply to its budget while the probe reports echoing', async () => {
vi.useFakeTimers()
const writes: string[] = []
const probe = scriptedEchoProbe('echoing')
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: probe,
write: (data) => writes.push(data),
onEmission: () => {}
})
expect(ingress.answerLiveQueryReply(OSC_COLOR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(0)
for (let i = 0; i < 8; i += 1) {
ingress.writeQueryReplyInOrder(CPR_REPLY)
}
// A burst of ordered replies must not exhaust the probe-start budget and force an
// early unprobed flush, which would paint the reply onto a still-cooked prompt.
await vi.advanceTimersByTimeAsync(199)
expect(writes).toEqual([])
await vi.advanceTimersByTimeAsync(2)
expect(writes[0]).toBe(OSC_COLOR_REPLY)
ingress.drainAndClose()
})
it('hands a queued ordered reply to the pty at teardown instead of dropping it', async () => {
vi.useFakeTimers()
const writes: string[] = []
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: () => new Promise(() => {}),
write: (data) => writes.push(data),
onEmission: () => {}
})
expect(ingress.answerLiveQueryReply(OSC_COLOR_REPLY)).toBe(true)
await vi.advanceTimersByTimeAsync(0)
expect(ingress.writeQueryReplyInOrder(CPR_REPLY)).toBe(true)
// Daemon dispose drains before the kill, so the child can still be blocked on this.
ingress.drainAndClose()
expect(writes).toEqual([CPR_REPLY])
})
it('never takes the ordered path on backends that do not defer', async () => {
vi.useFakeTimers()
for (const ownerBackend of ['windows-conpty', 'windows-wsl'] as const) {
const writes: string[] = []
const ingress = new PtyStartupIngress({
ownerBackend,
write: (data) => writes.push(data),
onEmission: () => {}
})
expect(ingress.answerLiveQueryReply(OSC_COLOR_REPLY)).toBe(true)
expect(ingress.writeQueryReplyInOrder(CPR_REPLY)).toBe(false)
expect(writes).toEqual([OSC_COLOR_REPLY])
ingress.drainAndClose()
}
})
it('does not route ordinary keystrokes through the echo-safe reply path', () => {
expect(needsCookedEchoSafeQueryReply('y')).toBe(false)
expect(needsCookedEchoSafeQueryReply('yes\r')).toBe(false)
+9 -8
View File
@@ -100,6 +100,11 @@ export class PtyStartupIngress {
: false
}
/** Order-only: holds an uncontained reply behind a deferred one, else leaves it. */
writeQueryReplyInOrder(reply: string): boolean {
return !this.closed && this.delivery.writeBehindDeferredReplies(reply)
}
drainAndClose(): number {
this.enqueue({ kind: 'teardown' })
return this.rawHighWater
@@ -322,12 +327,11 @@ export class PtyStartupIngress {
}
private releaseQueryPending(): void {
if (!this.queryPending) {
return
}
const pending = this.queryPending
this.queryPending = null
this.emit(pending, false)
if (pending) {
this.emit(pending, false)
}
}
/**
@@ -377,10 +381,7 @@ export class PtyStartupIngress {
}
private clearDeadline(): void {
if (!this.deadlineTimer) {
return
}
clearTimeout(this.deadlineTimer)
clearTimeout(this.deadlineTimer ?? undefined)
this.deadlineTimer = null
}
}
+58 -8
View File
@@ -74,7 +74,7 @@ const MAX_TRACKED_REPLIES = 64
const ECHO_PROBE_MAX_STARTS_PER_SECOND = 10
type ExpectedEcho = { projections: readonly string[]; remainingBytes: number }
type PendingWrite = { reply: string; onFailed: (() => void) | undefined }
type PendingWrite = { reply: string; onFailed: (() => void) | undefined; contained: boolean }
type ActiveEchoProbe = { timer: ReturnType<typeof setTimeout> | null }
/** Only a POSIX tty both echoes the reply and still delivers a deferred write. */
@@ -212,7 +212,31 @@ export class PtyStartupReplyDelivery {
if (this.pendingWrites.length === 0) {
this.echoPollDeadline = Date.now() + ECHO_POLL_BUDGET_MS
}
this.pendingWrites.push({ reply, onFailed })
this.pendingWrites.push({ reply, onFailed, contained: true })
this.armWriteTimer()
return true
}
/**
* Queues a reply that needs no echo containment behind ones that do, so it cannot
* overtake them. termenv writes `OSC 11 ;? ST` then `CSI 6n` and stops reading at
* the CPR, treating it as proof the terminal answered: a CPR that jumps a still-
* deferred color reply strands that reply in the tty for whatever runs next to read
* (`gh auth login` -> "unexpected escape sequence from terminal").
*
* False means nothing is deferred and the caller owns the write, which keeps the
* latency-critical replies immediate on every path that is not mid-deferral.
*/
writeBehindDeferredReplies(reply: string): boolean {
if (this.closed || this.pendingWrites.length === 0) {
return false
}
if (this.pendingWrites.length >= MAX_TRACKED_REPLIES) {
// Flushing keeps order without growing the queue; the caller writes after it.
this.flushPendingWrites()
return false
}
this.pendingWrites.push({ reply, onFailed: undefined, contained: false })
this.armWriteTimer()
return true
}
@@ -264,17 +288,32 @@ export class PtyStartupReplyDelivery {
}
}
/** Teardown: the pty is gone, so an unwritten reply has nowhere left to go. */
/**
* Teardown. A contained reply has nowhere left to go — its reader is gone. An
* uncontained one is an ordinary write this queue only borrowed for ordering, and the
* caller was already told it was sent, so it is handed to the pty best-effort: the
* child can still be alive here (daemon dispose drains before the kill).
*/
close(): void {
this.closed = true
this.clearWriteTimer()
this.clearActiveEchoProbe()
this.pendingWrites.length = 0
for (const pending of this.pendingWrites.splice(0)) {
if (!pending.contained) {
try {
this.writeProvider(pending.reply)
} catch {
/* pty already torn down */
}
}
}
this.expectedEchoes.length = 0
}
private armWriteTimer(delayMs = 0): void {
if (this.writeTimer) {
// A probe in flight is already the continuation: re-arming forks a second stty and
// makes the first one's verdict unusable (the identity bail below discards it).
if (this.writeTimer || this.activeEchoProbe) {
return
}
this.writeTimer = setTimeout(() => this.attemptPendingWrites(), delayMs)
@@ -332,7 +371,7 @@ export class PtyStartupReplyDelivery {
this.clearWriteTimer()
this.clearActiveEchoProbe()
for (const pending of this.pendingWrites.splice(0)) {
this.writeReply(pending.reply, pending.onFailed, kernelEchoImpossible)
this.writeReply(pending.reply, pending.onFailed, kernelEchoImpossible, pending.contained)
}
}
@@ -371,11 +410,22 @@ export class PtyStartupReplyDelivery {
return true
}
private writeReply(reply: string, onFailed?: () => void, kernelEchoImpossible = false): boolean {
private writeReply(
reply: string,
onFailed?: () => void,
kernelEchoImpossible = false,
contained = true
): boolean {
if (this.closed) {
return false
}
const projections = replyEchoProjections(reply, this.ownerBackend, kernelEchoImpossible)
// Not a safety rule: the caret form of a short CSI reply (`^[[6;1R`) is far likelier
// to occur verbatim in ordinary output than a long OSC colour string, so arming it
// is not worth the false swallow. These replies were unprojected before they rode
// this queue, and stay that way.
const projections = contained
? replyEchoProjections(reply, this.ownerBackend, kernelEchoImpossible)
: []
// Why: register before write because node-pty can synchronously re-enter onData.
const expected: ExpectedEcho | null =
projections.length > 0 ? { projections, remainingBytes: ECHO_SEARCH_BUDGET_BYTES } : null
+101 -2
View File
@@ -1,10 +1,12 @@
import { Terminal } from '@xterm/headless'
import { describe, expect, it } from 'vitest'
import { describe, expect, it, vi } from 'vitest'
import {
extractOnlyCookedEchoSafeQueryReplies,
isTerminalQueryReply,
needsCookedEchoSafeQueryReply
needsCookedEchoSafeQueryReply,
takeLiveQueryReply
} from './terminal-query-reply'
import { PtyStartupIngress } from './pty-startup-ingress'
describe('isTerminalQueryReply', () => {
it('matches synthetic query replies that must be sent immediately', () => {
@@ -125,3 +127,100 @@ describe('isTerminalQueryReply', () => {
expect(isTerminalQueryReply('\x1bP1$r2 q')).toBe(false)
})
})
// Regression for the `gh auth login` report: termenv writes `OSC 11 ;? ST` then
// `CSI 6n` and reads exactly one response, treating a CPR-first answer as "no OSC
// support" without draining further. Orca defers the color reply behind an ECHO
// probe, so a CPR taken straight to the PTY overtakes it and leaves `ESC ]` in the
// tty for the next program — bubbletea then dies with
// "unexpected escape sequence from terminal: ['\x1b' ']']".
describe('takeLiveQueryReply ordering (termenv OSC-then-CPR)', () => {
const OSC_11_REPLY = '\x1b]11;rgb:1e1e/1e1e/1e1e\x1b\\'
const CPR_REPLY = '\x1b[1;1R'
function hostWrites(ingress: PtyStartupIngress, pty: string[]) {
return (data: string): void => {
if (!takeLiveQueryReply(ingress, data)) {
pty.push(data)
}
}
}
it('delivers the color reply before the CPR the querying program stops at', async () => {
vi.useFakeTimers()
const pty: string[] = []
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: async () => 'quiet',
write: (data) => pty.push(data),
onEmission: () => {}
})
const write = hostWrites(ingress, pty)
write(OSC_11_REPLY)
write(CPR_REPLY)
await vi.advanceTimersByTimeAsync(200)
expect(pty).toEqual([OSC_11_REPLY, CPR_REPLY])
// Nothing survives the CPR, so the next program's stdin opens clean.
expect(pty.slice(1).join('')).not.toContain('\x1b]')
ingress.drainAndClose()
vi.useRealTimers()
})
it('keeps a CPR immediate when no color reply is deferred', () => {
vi.useFakeTimers()
const pty: string[] = []
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: async () => 'quiet',
write: (data) => pty.push(data),
onEmission: () => {}
})
hostWrites(ingress, pty)(CPR_REPLY)
expect(pty).toEqual([CPR_REPLY])
ingress.drainAndClose()
vi.useRealTimers()
})
it('never takes ordinary typed input', () => {
const pty: string[] = []
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
write: (data) => pty.push(data),
onEmission: () => {}
})
for (const keystroke of ['y', 'gh auth login\r', '\x1b[A', '\x1b', '\x03']) {
expect(takeLiveQueryReply(ingress, keystroke)).toBe(false)
}
ingress.drainAndClose()
})
})
// Known limitation, tracked for follow-up: the guarantee is FIFO among recognised
// query replies, not over every byte written to the pty. Both branches of
// takeLiveQueryReply match whole strings, so a coalesced payload is not a reply, and
// ordinary input never rides the queue at all. Pinned so the boundary is explicit —
// if a change makes these take the ordered path, that is an improvement, not a break.
describe('takeLiveQueryReply: writes that still bypass the ordered queue', () => {
const BYPASSING = [
{ what: 'reply coalesced with a keystroke', data: '\x1b[6;1Ry' },
{ what: 'keystroke coalesced with a reply', data: 'y\x1b[6;1R' },
{ what: 'ordinary typed input', data: 'ls\r' }
]
it.each(BYPASSING)('does not take $what', ({ data }) => {
const ingress = new PtyStartupIngress({
ownerBackend: 'posix-pty',
echoProbe: () => new Promise(() => {}),
write: () => {},
onEmission: () => {}
})
// Deferral is open, so an ordered write WOULD be queued here.
expect(ingress.answerLiveQueryReply('\x1b]11;rgb:00/00/00\x07')).toBe(true)
expect(takeLiveQueryReply(ingress, data)).toBe(false)
ingress.drainAndClose()
})
})
+31 -4
View File
@@ -23,10 +23,12 @@ const ESC = String.fromCharCode(0x1b)
/* oxlint-disable no-control-regex -- grammars match terminal ESC/BEL sequences by definition */
// Known accepted collision: xterm.js encodes MODIFIED F3 (Shift/Ctrl/Alt+F3) as
// `CSI 1 ; <mod> R`, which is indistinguishable from a CPR report (a classic
// VT ambiguity). Such a keystroke is sent immediately instead of debounced —
// byte order is still preserved (the immediate path flushes pending input
// first), it just skips input-intent/activity bookkeeping. Harmless, so we
// keep the reply grammar complete rather than special-casing it.
// VT ambiguity). Such a keystroke skips input-intent/activity bookkeeping, and
// takeLiveQueryReply will hold it behind a deferred reply like any CPR — so
// within that window (bounded by the host's echo budget) a keystroke typed
// after it can reach the pty first. Accepted: it needs the chord and a
// following keypress inside a live colour-query deferral, and we keep the reply
// grammar complete rather than special-casing an unresolvable ambiguity.
const CPR_OR_DSR_RE = new RegExp('^\\u001b\\[\\??[0-9;]*[Rn]$')
const DEVICE_ATTRIBUTES_RE = new RegExp('^\\u001b\\[[?>=]?[0-9;]*c$')
// 4/6 = pixel-size reports, 8 = text-area size in characters (answer to CSI 18t).
@@ -144,3 +146,28 @@ export function answerEachCookedEchoSafeQueryReply(
}
return accepted
}
/** Host-side ingress surface for replies the PTY owner may take ownership of. */
export type LiveQueryReplyIngress = {
answerLiveQueryReply: (reply: string) => boolean
writeQueryReplyInOrder: (reply: string) => boolean
}
/**
* True when `ingress` owns this write. A reply a cooked prompt would paint is deferred
* behind an ECHO probe (#13137); a latency-critical reply (CPR/DA) is taken only while
* one of those is still deferred, because termenv stops reading at the CPR it sends
* after its color query — an early CPR strands the color reply in the tty for whatever
* runs next (`gh auth login` -> "unexpected escape sequence from terminal").
*/
export function takeLiveQueryReply(
ingress: LiveQueryReplyIngress | undefined,
data: string
): boolean {
if (!ingress) {
return false
}
return extractOnlyCookedEchoSafeQueryReplies(data)
? ingress.answerLiveQueryReply(data)
: isTerminalQueryReply(data) && ingress.writeQueryReplyInOrder(data)
}