diff --git a/src/main/daemon/degraded-daemon-pty-provider.ts b/src/main/daemon/degraded-daemon-pty-provider.ts index 4cd907687bd..2b50424ae83 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.ts @@ -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 { - 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 { diff --git a/src/main/ipc/pty-controller-ownership-routing.test.ts b/src/main/ipc/pty-controller-ownership-routing.test.ts index 9fca68cedde..cf61b71c9e5 100644 --- a/src/main/ipc/pty-controller-ownership-routing.test.ts +++ b/src/main/ipc/pty-controller-ownership-routing.test.ts @@ -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 +} 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 - } + 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 + // 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) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index d08e437add3..f50ad36d34c 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -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) } diff --git a/src/main/providers/provider-dispatch.test.ts b/src/main/providers/provider-dispatch.test.ts index cf2d9979a88..9c40b3850eb 100644 --- a/src/main/providers/provider-dispatch.test.ts +++ b/src/main/providers/provider-dispatch.test.ts @@ -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(), diff --git a/src/main/providers/pty-provider-contract.ts b/src/main/providers/pty-provider-contract.ts index 981ffad9a9c..9adaa580e6a 100644 --- a/src/main/providers/pty-provider-contract.ts +++ b/src/main/providers/pty-provider-contract.ts @@ -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 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 + /** 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 resize(id: string, cols: number, rows: number): void /** * Producer-side flow control: stop/restart reading the underlying PTY so a diff --git a/src/main/providers/settled-pty-writer-census.test.ts b/src/main/providers/settled-pty-writer-census.test.ts new file mode 100644 index 00000000000..845c4422935 --- /dev/null +++ b/src/main/providers/settled-pty-writer-census.test.ts @@ -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\(/) + } + }) +})