mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
NeilandBrennan
bc319abfbe
Root-cause follow-up to #15559, which stopped a CPR overtaking a deferred colour reply but left the deferral itself in place. Orca answers terminal queries by writing to the PTY master, which a line discipline in ECHO copies straight back out as junk on a cooked prompt (#12112). The guard was to withhold the write until an `stty` subprocess proved ECHO clear — and forking is what forced the decision to be async. Any deferral, however short, lets a reply written later in the same turn overtake this one, so the async probe was the bug's root cause. Read the bit synchronously instead. Linux and the BSDs redirect a master's mode ioctls to the slave, so a `tcgetattr` on the master fd node-pty already owns answers for the slave with no fork: measured 0.26us against 2403us for the subprocess. With a verdict available inline, a querying program that already cleared ECHO — every raw-mode prober, including the colour probe behind the `gh auth login` report — is answered in its own turn and can never be reordered. The deferral stays for the genuinely cooked case, and the ordering guarantee stays underneath it: hosts whose node-pty predates this patch get no sync probe and fall back to the deferred path, which mixed client/host versions make a live production path. Reply routing is all-or-nothing: a payload needing neither containment nor ordering stays on the host's own path, so a CPR answered during shell startup cannot pass the daemon's post-ready flush gate and splice into the buffered startup command. Native side is fail-safe: a kernel that did not redirect would answer from the master's own termios, whose ECHO defaults set, so the degraded verdict is "echoing" — never a false "quiet". The JS half ships in the pnpm patch while the binding needs a source build, so ORCA_REQUIRE_NODE_PTY_ECHO_STATE=1 makes CI fail rather than silently skip when it is handed an upstream prebuild. Co-authored-by: Brennan <brennanb2025@users.noreply.github.com>
68 lines
3.2 KiB
TypeScript
68 lines
3.2 KiB
TypeScript
/**
|
|
* Decides whether the loaded node-pty really carries Orca's `echoState` binding, and
|
|
* how strict the suites that depend on it are about its absence.
|
|
*
|
|
* Why this is not "is the sync probe defined": the JS half of the patch ships in the
|
|
* pnpm patch, so `createPtySlaveEchoSyncProbe` hands back a probe as soon as
|
|
* node_modules is patched — even when the loaded NATIVE module is the upstream
|
|
* prebuild, whose exports are ['fork', 'open', 'resize', 'process']. `readEchoState()`
|
|
* then answers undefined, the probe maps it to 'unknown', #13892's same-turn fast path
|
|
* never fires, and a suite that only checked the probe exists would pass with the fix
|
|
* switched off.
|
|
*/
|
|
|
|
/** Env var CI sets to make a prebuilt or stale node-pty fail instead of skip. */
|
|
export const REQUIRE_NODE_PTY_ECHO_STATE_ENV_VAR = 'ORCA_REQUIRE_NODE_PTY_ECHO_STATE'
|
|
|
|
/** Spelled out because `pnpm rebuild node-pty` finds the shipped darwin prebuild and exits 0. */
|
|
export const NODE_PTY_SOURCE_BUILD_HINT =
|
|
'Build node-pty from source with `npm_config_build_from_source=true pnpm rebuild node-pty` or `node config/scripts/rebuild-native-deps.mjs`; plain `pnpm rebuild node-pty` is not enough on macOS, where it finds the shipped darwin prebuild and exits 0.'
|
|
|
|
export type NodePtyEchoStateLookup =
|
|
| { available: true; nativeExports: readonly string[] }
|
|
| { available: false; nativeExports: readonly string[]; reason: string }
|
|
|
|
/** Pass the already-imported `node-pty` module; it exposes the raw binding as `native`. */
|
|
export function resolveNodePtyEchoStateSupport(
|
|
nodePtyModule: unknown,
|
|
platform: NodeJS.Platform = process.platform
|
|
): NodePtyEchoStateLookup {
|
|
const native = (nodePtyModule as { native?: unknown } | null | undefined)?.native
|
|
if (typeof native !== 'object' || native === null) {
|
|
return {
|
|
available: false,
|
|
nativeExports: [],
|
|
reason:
|
|
platform === 'win32'
|
|
? 'ConPTY has no slave line discipline to read, so node-pty exposes no native binding'
|
|
: `node-pty exposed no native binding to read echoState from. ${NODE_PTY_SOURCE_BUILD_HINT}`
|
|
}
|
|
}
|
|
const nativeExports = Object.keys(native as Record<string, unknown>)
|
|
if (typeof (native as Record<string, unknown>).echoState !== 'function') {
|
|
return {
|
|
available: false,
|
|
nativeExports,
|
|
reason: `the loaded node-pty native binding exports [${nativeExports.join(', ')}] and has no echoState, so it is an upstream prebuild rather than a build of Orca's patch — every sync probe would answer 'unknown' and #13892's same-turn reply would be off. ${NODE_PTY_SOURCE_BUILD_HINT}`
|
|
}
|
|
}
|
|
return { available: true, nativeExports }
|
|
}
|
|
|
|
/**
|
|
* The message to fail with when CI demanded a source-built node-pty and did not get
|
|
* one, else null.
|
|
*
|
|
* Assert this in a test that always runs, so the requirement cannot vanish with the
|
|
* suite it guards.
|
|
*/
|
|
export function nodePtyEchoStateRequirementViolation(
|
|
lookup: NodePtyEchoStateLookup,
|
|
env: NodeJS.ProcessEnv = process.env
|
|
): string | null {
|
|
if (lookup.available || env[REQUIRE_NODE_PTY_ECHO_STATE_ENV_VAR] !== '1') {
|
|
return null
|
|
}
|
|
return `${REQUIRE_NODE_PTY_ECHO_STATE_ENV_VAR}=1 but the patched-node-pty tests would skip: ${lookup.reason}`
|
|
}
|