mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 08:03:20 +00:00
fix(pty): require a settled writer on every PTY provider
The production controller never installed `writeWithSettlement`, so every mailbox
pointer write took the fire-and-forget fallback and provider settlement never
controlled the pointer transition (R3-C2). Making the method required is what
turns that into a compile error, so `IPtyProvider.writeWithSettlement` is now
non-optional and `LocalPtyProvider` implements it synchronously: in-process
node-pty is its own sole owner, so its answer is the settlement.
Required-ness alone is satisfiable by a lie. `degraded-daemon-pty-provider` used
to answer it with `provider.write(...) !== false`, which reproduces the bug
through the fix, so its conditional is deleted outright and it delegates.
`RuntimePtyController.writeWithSettlement` stays optional: 809 `setPtyController`
call sites across 164 files construct partial controllers, and requiring it there
would be a mechanical edit of all of them for no added fence. Instead the
orchestration pointer path demands the narrow settled-writer shape and returns
`refused{provider_cannot_settle}` before any effect when it is absent.
Two gates, both red against the fabricated handoff:
- `registerPtyHandlers` with a real controller and a provider lacking settlement
refuses before any byte reaches `provider.write`.
- A census pins the five production `IPtyProvider` classes, proves each instance
exposes the writer, and reads every declaring body to reject a settlement
synthesized from `.write(`.
This commit is contained in:
@@ -19,7 +19,7 @@ import {
|
||||
} from './degraded-daemon-session-routing'
|
||||
import { DegradedDaemonFreshSpawnRouter } from './degraded-daemon-fresh-spawn-routing'
|
||||
import { DegradedDaemonOwnerRecovery } from './degraded-daemon-owner-recovery'
|
||||
import { writeRefused, type WriteSettlement } from '../../shared/pty-write-settlement'
|
||||
import type { WriteSettlement } from '../../shared/pty-write-settlement'
|
||||
|
||||
export class DegradedDaemonPtyProvider implements IPtyProvider {
|
||||
readonly isDegraded = true
|
||||
@@ -114,10 +114,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider {
|
||||
}
|
||||
|
||||
async writeWithSettlement(id: string, data: string): Promise<WriteSettlement> {
|
||||
const provider = this.providerFor(id)
|
||||
return provider.writeWithSettlement
|
||||
? await provider.writeWithSettlement(id, data)
|
||||
: writeRefused('provider_cannot_settle')
|
||||
return await this.providerFor(id).writeWithSettlement(id, data)
|
||||
}
|
||||
|
||||
resize(id: string, cols: number, rows: number): void {
|
||||
|
||||
@@ -12,6 +12,15 @@ import {
|
||||
setLocalPtyProvider,
|
||||
unregisterSshPtyProvider
|
||||
} from './pty'
|
||||
import {
|
||||
writeRefused,
|
||||
writeUnverifiable,
|
||||
type WriteSettlement
|
||||
} from '../../shared/pty-write-settlement'
|
||||
|
||||
type SettledControllerDouble = {
|
||||
writeWithSettlement: (id: string, data: string) => WriteSettlement | Promise<WriteSettlement>
|
||||
}
|
||||
|
||||
vi.mock('electron', () => import('./pty-ipc-mock-registry').then((m) => m.electronModuleMock()))
|
||||
vi.mock('fs', () => import('./pty-ipc-mock-registry').then((m) => m.fsModuleMock()))
|
||||
@@ -99,17 +108,17 @@ describe('registerPtyHandlers', () => {
|
||||
const ptyId = `ssh:${connectionId}@@remote-pty`
|
||||
const provider = {
|
||||
...createAgentClaimProvider({}),
|
||||
writeWithSettlement: vi.fn().mockRejectedValue(new Error('connection lost after write'))
|
||||
writeWithSettlement: vi
|
||||
.fn()
|
||||
.mockResolvedValue(writeUnverifiable('transport_settlement_lost', true))
|
||||
}
|
||||
registerSshPtyProvider(connectionId, provider as never)
|
||||
setPtyOwnership(ptyId, connectionId)
|
||||
const controller = registerAgentClaimController() as unknown as {
|
||||
writeWithSettlement: (id: string, data: string) => Promise<boolean>
|
||||
}
|
||||
const controller = registerAgentClaimController() as unknown as SettledControllerDouble
|
||||
try {
|
||||
expect(controller.writeWithSettlement).toBeTypeOf('function')
|
||||
await expect(controller.writeWithSettlement(ptyId, 'pointer')).rejects.toThrow(
|
||||
'connection lost after write'
|
||||
await expect(controller.writeWithSettlement(ptyId, 'pointer')).resolves.toEqual(
|
||||
writeUnverifiable('transport_settlement_lost', true)
|
||||
)
|
||||
expect(provider.writeWithSettlement).toHaveBeenCalledWith(ptyId, 'pointer')
|
||||
expect(provider.write).not.toHaveBeenCalled()
|
||||
@@ -119,6 +128,27 @@ describe('registerPtyHandlers', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('refuses a settled write before any bytes when the routed provider cannot settle', async () => {
|
||||
const connectionId = 'ssh-unsettled'
|
||||
const ptyId = `ssh:${connectionId}@@remote-pty`
|
||||
const provider = createAgentClaimProvider({}) as Record<string, unknown>
|
||||
// A provider predating the settled contract, reached through the production registry.
|
||||
delete provider.writeWithSettlement
|
||||
registerSshPtyProvider(connectionId, provider as never)
|
||||
setPtyOwnership(ptyId, connectionId)
|
||||
const controller = registerAgentClaimController() as unknown as SettledControllerDouble
|
||||
try {
|
||||
// Synchronous by construction: the refusal happens before any effect is attempted.
|
||||
expect(await controller.writeWithSettlement(ptyId, 'pointer')).toEqual(
|
||||
writeRefused('provider_cannot_settle')
|
||||
)
|
||||
expect(provider.write).not.toHaveBeenCalled()
|
||||
} finally {
|
||||
unregisterSshPtyProvider(connectionId)
|
||||
clearProviderPtyState(ptyId)
|
||||
}
|
||||
})
|
||||
|
||||
it('preserves a provider write refusal for callers that gate follow-up input', () => {
|
||||
const provider = createAgentClaimProvider({})
|
||||
provider.write.mockReturnValue(false)
|
||||
|
||||
@@ -1,5 +1,10 @@
|
||||
import type * as pty from 'node-pty'
|
||||
import type { IPtyProvider, PtyProcessInfo, PtySpawnOptions, PtySpawnResult } from './types'
|
||||
import {
|
||||
WRITE_ACCEPTED,
|
||||
writeRefused,
|
||||
type WriteSettlement
|
||||
} from '../../shared/pty-write-settlement'
|
||||
import {
|
||||
confirmLocalPtyForegroundProcess,
|
||||
confirmLocalPtyShellForeground,
|
||||
@@ -73,6 +78,11 @@ export class LocalPtyProvider implements IPtyProvider {
|
||||
write(id: string, data: string): boolean {
|
||||
return writeLocalPty(id, data)
|
||||
}
|
||||
|
||||
// In-process node-pty is its own sole owner, so its synchronous answer is the settlement.
|
||||
writeWithSettlement(id: string, data: string): WriteSettlement {
|
||||
return writeLocalPty(id, data) ? WRITE_ACCEPTED : writeRefused('provider_refused_write')
|
||||
}
|
||||
resize(id: string, cols: number, rows: number): void {
|
||||
resizeLocalPty(id, cols, rows)
|
||||
}
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { settledWriteStub } from './settled-pty-write-stub'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { setPtyHostBindings } from '../ipc/pty-host-bindings'
|
||||
|
||||
@@ -101,6 +102,7 @@ describe('PTY provider dispatch', () => {
|
||||
spawn: vi.fn().mockResolvedValue({ id }),
|
||||
attach: vi.fn(),
|
||||
write: vi.fn(),
|
||||
writeWithSettlement: vi.fn(settledWriteStub()),
|
||||
resize: vi.fn(),
|
||||
shutdown: vi.fn(),
|
||||
sendSignal: vi.fn(),
|
||||
|
||||
@@ -141,8 +141,10 @@ export type IPtyProvider = {
|
||||
/** Exact provider readback: false only when the provider answered that the PTY is absent. */
|
||||
probePtyLiveness?: (id: string) => Promise<boolean | null>
|
||||
write(id: string, data: string): boolean | void
|
||||
/** Three-valued settlement for writes whose delivery a durable claim depends on. */
|
||||
writeWithSettlement?: (id: string, data: string) => WriteSettlement | Promise<WriteSettlement>
|
||||
/** Three-valued settlement for writes whose delivery a durable claim depends on.
|
||||
* Required: a provider that answers this from its own fire-and-forget `write` is
|
||||
* fabricating a handoff, so every provider must settle or say it cannot. */
|
||||
writeWithSettlement: (id: string, data: string) => WriteSettlement | Promise<WriteSettlement>
|
||||
resize(id: string, cols: number, rows: number): void
|
||||
/**
|
||||
* Producer-side flow control: stop/restart reading the underlying PTY so a
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
import { readFileSync } from 'node:fs'
|
||||
import { join } from 'node:path'
|
||||
import { execFileSync } from 'node:child_process'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { LocalPtyProvider } from './local-pty-provider'
|
||||
import { SshPtyProvider } from './ssh-pty-provider'
|
||||
import { createMockMux } from './ssh-pty-provider-mock-multiplexer'
|
||||
import { DaemonPtyRouter } from '../daemon/daemon-pty-router'
|
||||
import { DegradedDaemonPtyProvider } from '../daemon/degraded-daemon-pty-provider'
|
||||
import { DaemonPtyAdapter } from '../daemon/daemon-pty-adapter'
|
||||
|
||||
vi.mock('electron', () => ({
|
||||
app: { getPath: vi.fn(() => '/tmp'), isPackaged: false },
|
||||
BrowserWindow: { fromId: vi.fn(() => null) },
|
||||
ipcMain: { on: vi.fn(), removeListener: vi.fn() },
|
||||
webContents: { fromId: vi.fn(() => null) }
|
||||
}))
|
||||
|
||||
const REPO_ROOT = join(__dirname, '..', '..', '..')
|
||||
|
||||
/**
|
||||
* Requiring the method is satisfiable by a lie: the degraded daemon router used to answer it
|
||||
* with `provider.write(...) !== false`, reproducing the fire-and-forget bug through the fix.
|
||||
* The census pins the producers and reads their bodies, so a new provider or a revived
|
||||
* fabricated handoff fails here rather than silently clearing a mailbox reservation.
|
||||
*/
|
||||
const SETTLED_PTY_WRITER_FILES = [
|
||||
'src/main/providers/local-pty-provider.ts',
|
||||
'src/main/providers/ssh-pty-provider.ts',
|
||||
'src/main/daemon/daemon-pty-router.ts',
|
||||
'src/main/daemon/degraded-daemon-pty-provider.ts',
|
||||
'src/main/daemon/daemon-pty-adapter.ts'
|
||||
]
|
||||
|
||||
/** Where the provider-side settlement is actually decided; the adapter inherits its own. */
|
||||
const SETTLED_WRITER_DECLARATIONS = [
|
||||
'src/main/providers/local-pty-provider.ts',
|
||||
'src/main/providers/ssh-pty-provider.ts',
|
||||
'src/main/providers/ssh-pty-provider-rpc-operations.ts',
|
||||
'src/main/daemon/daemon-pty-router.ts',
|
||||
'src/main/daemon/degraded-daemon-pty-provider.ts',
|
||||
'src/main/daemon/daemon-pty-session-input.ts'
|
||||
]
|
||||
|
||||
function declaredProviderFiles(): string[] {
|
||||
const output = execFileSync('git', ['grep', '-l', '--', 'implements IPtyProvider', 'src/main'], {
|
||||
cwd: REPO_ROOT,
|
||||
encoding: 'utf8'
|
||||
})
|
||||
return output.split('\n').filter(Boolean).sort()
|
||||
}
|
||||
|
||||
function settledWriterBody(file: string): string {
|
||||
const source = readFileSync(join(REPO_ROOT, file), 'utf8')
|
||||
const start = source.indexOf('writeWithSettlement')
|
||||
expect(start, `${file} declares no settled writer`).toBeGreaterThan(-1)
|
||||
const end = source.indexOf('\n }', start)
|
||||
return source.slice(start, end === -1 ? source.length : end)
|
||||
}
|
||||
|
||||
describe('settled PTY writer census', () => {
|
||||
it('covers every production provider class that declares IPtyProvider', () => {
|
||||
expect(declaredProviderFiles()).toEqual([...SETTLED_PTY_WRITER_FILES].sort())
|
||||
})
|
||||
|
||||
it('exposes a settled writer on every production provider instance', () => {
|
||||
const daemonClient = { isConnected: () => false, onEvent: vi.fn(() => vi.fn()) }
|
||||
const adapter = new DaemonPtyAdapter(daemonClient as never)
|
||||
const instances = [
|
||||
new LocalPtyProvider({} as never),
|
||||
new SshPtyProvider('conn-census', createMockMux() as never),
|
||||
new DaemonPtyRouter({ current: adapter, legacy: [] }),
|
||||
new DegradedDaemonPtyProvider({
|
||||
current: adapter,
|
||||
legacy: [],
|
||||
fallback: new LocalPtyProvider({} as never)
|
||||
}),
|
||||
adapter
|
||||
]
|
||||
for (const provider of instances) {
|
||||
expect(typeof provider.writeWithSettlement, provider.constructor.name).toBe('function')
|
||||
}
|
||||
})
|
||||
|
||||
it('never synthesizes a settlement from the fire-and-forget write', () => {
|
||||
for (const file of SETTLED_WRITER_DECLARATIONS) {
|
||||
expect(settledWriterBody(file), file).not.toMatch(/\.write\(/)
|
||||
}
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user